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.

Sign up for GitHub

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

Jump to bottom

Change GetUnique() return type from char to init8_t #1269

Closed
wants to merge 2 commits into from
Closed

Change GetUnique() return type from char to init8_t #1269

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bbf2c5
Change GetUnique() return type from char to init8_t
martin-g Apr 4, 2024
c9e4223
Use 0xFF instead of -1 for the unique mask
martin-g Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/common/kmer_index/extension_index/inout_mask.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ class InOutMask {
return unique[mask];
}

static constexpr char GetUnique(uint8_t mask) {
constexpr char next[] =
static constexpr int8_t GetUnique(uint8_t mask) {
Copy link
Member

@asl asl Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the change! However, I think we need to go the other way. Here is the full story:

  • The return value is assumed to be 0 - 3 (representing A, C, G, T correspondingly) for input of 0, 1, 2, 4 (converting single-bit mask to a nucleotide)
  • -1's represent the impossible values, but still they should be as initializers.

So, the return type should really be char as it is used later on in GetUniqueOutgoing / GetUniqueIncoming.

I would probably use 0xFF instead of -1. Does this work for you on aarch64-linux? To make things explicit we can even do something like:

                {  UINT8_C(0xFF),  0,  1, UINT8_C(0xFF),  2, UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF),
                  3, UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF), UINT8_C(0xFF) };

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! That works too!
Let me update the PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated it but I have the feeling that more changes are needed at the call sites of GetUniqueOutgoing / GetUniqueIncoming.

Copy link
Member

Choose a reason for hiding this comment

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

Callsites should be fine. The contract is that one should never call GetUniqueOutgoing / GetUniqueIncoming if they could return 0xFF.

constexpr int8_t next[] =
{ -1, 0, 1, -1, 2, -1, -1, -1,
3, -1, -1, -1, -1, -1, -1, -1 };
return next[mask];
Expand Down