Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expand Indexlr to use AAHash #109

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Expand Indexlr to use AAHash #109

wants to merge 12 commits into from

Conversation

jwcodee
Copy link
Member

@jwcodee jwcodee commented Jul 16, 2023

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

@jwcodee jwcodee requested review from vlad0x00, lcoombe and parham-k July 16, 2023 02:21
@@ -22,6 +23,77 @@

namespace btllib {

struct Minimizer
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@jwcodee jwcodee Jul 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Minimizer doesn't need a namespace and I think we can rename Record to IndexlrRecord to be specific. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about nested namespaces? Like btllib::indexlr, and put Record under btllib::indexlr?

@@ -141,6 +141,12 @@ class AAHash
size_t get_pos() const { return pos; }
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function is redundant and removed in the latest ntHash. We can remove it here too.

@@ -22,6 +23,77 @@

namespace btllib {

struct Minimizer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants