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

Fix uninitialized effective lengths for ref. #64

Merged
merged 2 commits into from
Jan 28, 2022
Merged

Conversation

haowenz
Copy link
Owner

@haowenz haowenz commented Jan 28, 2022

@mourisl Please be more careful when adding uninitialized variables as they are dangerous and cause issues that are not easy to debug.

@haowenz haowenz added the bug Something isn't working label Jan 28, 2022
@haowenz haowenz requested a review from mourisl January 28, 2022 15:59
@mourisl
Copy link
Collaborator

mourisl commented Jan 28, 2022

effective_range_[0] = 0;
effective_range_[1] = -1;
effective_range_[2] = 1;

I did the initialization in the constructor function here. Not sure why the fix works, and there might be another reason for #62 ?

@haowenz
Copy link
Owner Author

haowenz commented Jan 28, 2022

You are right. To me the bug was likely caused by efffective_length[1]=0, which makes the length of sequences 0, thereby getting 0 minimizers. Otherwise, the code would fail somewhere else. As no others has had this issue, I think it is possible that somehow this user's compiler has a bug and did not call the default constructor. We can keep this open for a while and see.

@haowenz
Copy link
Owner Author

haowenz commented Jan 28, 2022

Did a bit more search and found a interesting post here. It is likely that in some rare case with some compilers the default constructor is somehow not called. Let us merge this and later refactor this class to use initializer list later.

@haowenz haowenz merged commit 1868ddf into master Jan 28, 2022
@haowenz haowenz deleted the fix-effective-length branch January 28, 2022 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants