Skip to content

util::find() issue fixed#9

Merged
jermp merged 2 commits intojermp:masterfrom
Razdeep:bug_fix
Feb 8, 2022
Merged

util::find() issue fixed#9
jermp merged 2 commits intojermp:masterfrom
Razdeep:bug_fix

Conversation

@Razdeep
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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