Divsufsort made header-only #45
Conversation
cpockrandt
force-pushed
the
divsufsort
branch
2 times, most recently
from
63292ef to
0955cef
Compare
There was a problem hiding this comment.
I think the code is still super hard to read with all the macros. I would propose to decide which of the configurables you want to preserve and put them in the config and remove the rest. Also you rely on BUILD_DIVSUFSORT64 in multiple places, but it is never set. Should this instead depend on saidx_t?
include/sdsl/divsufsort.hpp
Outdated
|
|
||
| namespace sdsl { | ||
|
|
||
| #define PROJECT_VERSION_FULL "2.0.1-15-g22e6b23" |
| #define SS_INSERTIONSORT_THRESHOLD (8) | ||
| #define SS_BLOCKSIZE (1024) | ||
| #define SS_MISORT_STACKSIZE (16) | ||
| #define TR_INSERTIONSORT_THRESHOLD (8) |
There was a problem hiding this comment.
They don't depend on 32/64 bit versions. We could move them in a separate config struct. But I only want to change the code when necessary.
| #else | ||
| #define BUCKET_B(_c0, _c1) (bucket_B[(_c1) * ALPHABET_SIZE + (_c0)]) | ||
| #define BUCKET_BSTAR(_c0, _c1) (bucket_B[(_c0) * ALPHABET_SIZE + (_c1)]) | ||
| #endif |
There was a problem hiding this comment.
Can you not just insert the actual code instead? Or make inline functions that do this?
There was a problem hiding this comment.
Then we should move the stack operations to functions as well. But the stack macro functions work with all combinations of parameters (values and pointers) at different lines of codes depending on the stack. Don't really want to start touching all that code. 👀
| do {\ | ||
| stack[ssize].a = (_a), stack[ssize].b = (_b),\ | ||
| stack[ssize].c = (_c), stack[ssize++].d = (_d);\ | ||
| } while(0) |
| 7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7,7 | ||
| }; | ||
|
|
||
| #if (SS_BLOCKSIZE == 0) || (SS_INSERTIONSORT_THRESHOLD < SS_BLOCKSIZE) |
There was a problem hiding this comment.
Both macros are fixed values in this file so either this condition is always true or always false
There was a problem hiding this comment.
I'd keep this as well just in case.
include/sdsl/divsufsort.hpp
Outdated
| int32_t | ||
| ss_ilg(saidx_t n) { | ||
| #if SS_BLOCKSIZE == 0 | ||
| # if defined(BUILD_DIVSUFSORT64) |
There was a problem hiding this comment.
this is never defined so the block is never evaluated
include/sdsl/divsufsort.hpp
Outdated
|
|
||
| /*---------------------------------------------------------------------------*/ | ||
|
|
||
| #if (SS_BLOCKSIZE != 1) && (SS_INSERTIONSORT_THRESHOLD != 1) |
There was a problem hiding this comment.
again, please remove dead code and/or checks as it will likely reduce the code considerably and make it easier to maintain
There was a problem hiding this comment.
I suggest keeping it for now (in case someone wants to take a look at it later and tune parameters). If I remove all the code now, you'd have to start from scratch again.
include/sdsl/divsufsort.hpp
Outdated
| /*---------------------------------------------------------------------------*/ | ||
|
|
||
| template <typename saidx_t> | ||
| static |
There was a problem hiding this comment.
don't mark any of the functions static (they are free functions anyway), but do mark all of them as inline!
include/sdsl/divsufsort.hpp
Outdated
| } | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
search and replace the multiple empty lines maybe 💅
|
is it possible to merge the code of divsufsort and divsufsort64? |
|
I'll add some test cases and after merging #44 this can be merged in my opinion. |
|
Tests are added and are passing. @mpetri From my point of view this PR is in a mergable state now. Or did you mean something different with your last question? |
This should not be merged before merging #44 (cmake/ctest refactoring) so Travis runs the full test suite.
@h-2 feel free to start reviewing :)