Conversation
|
|
||
| namespace btllib { | ||
|
|
||
| struct Minimizer |
There was a problem hiding this comment.
I am thinking of moving the flags out too and maybe put them under a namespace indexlrutilities. I think we can also have namespace with the same name as a class so name so we can make another namespace called indexlr too.
There was a problem hiding this comment.
Namespaces are generally reserved for a conceptually larger group of code than, in this case, a tool within a library, so I'd advise using something else.
There was a problem hiding this comment.
I think Minimizer doesn't need a namespace and I think we can rename Record to IndexlrRecord to be specific. Thoughts?
There was a problem hiding this comment.
How about nested namespaces? Like btllib::indexlr, and put Record under btllib::indexlr?
| unsigned get_hash_num() const { return hash_num; } | ||
| unsigned get_k() const { return k; } | ||
| bool | ||
| forward() // NOLINT(readability-convert-member-functions-to-static,-warnings-as-errors) |
There was a problem hiding this comment.
This function is redundant and removed in the latest ntHash. We can remove it here too.
|
|
||
| namespace btllib { | ||
|
|
||
| struct Minimizer |
There was a problem hiding this comment.
How about nested namespaces? Like btllib::indexlr, and put Record under btllib::indexlr?
| uint64_t out_hash, | ||
| size_t pos, | ||
| bool forward, | ||
| std::string seq) |
There was a problem hiding this comment.
I think having std::string as a argument here will trigger a string copy and affect performance, even though we're using move semantics in assignment (not sure but a copy of seq will be moved to this->seq?). Anyway, I'd use std::string_view, or just a const reference.
3 participants
Changes to every file but indexlr.cpp are complete. As indexlr.cpp require more complicated c++ changes, I want to request code review and guidance before I incorporate anymore changes