Skip to content

util::find() issue fixed #9

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

Merged
merged 2 commits into from
Feb 8, 2022
Merged

util::find() issue fixed #9

merged 2 commits into from
Feb 8, 2022

Conversation

Razdeep
Copy link
Contributor

@Razdeep Razdeep commented Feb 7, 2022

When linear scanning doesn't find the target element, the control must
not go back to the binary search logic.

Signed-off-by: Rajdeep Roy Chowdhury rrajdeeproychowdhury@gmail.com

When linear scanning doesn't find the target element, the control must
not go back to the binary search logic.

Signed-off-by: Rajdeep Roy Chowdhury <rrajdeeproychowdhury@gmail.com>
@Razdeep
Copy link
Contributor Author

Razdeep commented Feb 7, 2022

In this line here, when pos = 0, the value of hi is changed to std::numeric_limits<uint64_t>::max() since the datatype is unsigned. Is there a better way to handle such scenarios?

@jermp
Copy link
Owner

jermp commented Feb 7, 2022

Hi,
yes I think you are right. Indeed in the next_geq code below there is the break.
Can you check why the tests are failing then?
Thanks!

Signed-off-by: Rajdeep Roy Chowdhury <rrajdeeproychowdhury@gmail.com>
@jermp
Copy link
Owner

jermp commented Feb 8, 2022

How about:
return global::not_found; instead of break and leave the code below unchanged? (It saves the if and the logic is cleaner.)

@Razdeep
Copy link
Contributor Author

Razdeep commented Feb 8, 2022

Returning global::not_found or breaking; both of them are failing the unit test.
However, if I keep only the if logic below, the unit test passes and takes care of the unsigned underflow bug too.

@jermp
Copy link
Owner

jermp commented Feb 8, 2022

Oh yes, sorry the break of the return are just the same: I parsed one parenthesis less :)
Just setting hi = pos; and nothing else fixes the problem, without the if nor break.
Can you confirm this?

@Razdeep
Copy link
Contributor Author

Razdeep commented Feb 8, 2022

Setting hi = pos, introduces a potential infinity loop when pos is 0 and since the while loop condition is lo <= hi.
Changing the while condition from lo <= hi to lo < hi also breaks the unit tests.

@jermp jermp merged commit 21f10ad into jermp:master Feb 8, 2022
@jermp
Copy link
Owner

jermp commented Feb 8, 2022

Merged! Thanks.

aawadall pushed a commit to aawadall/autocomplete that referenced this pull request May 25, 2025
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.

2 participants