-
Notifications
You must be signed in to change notification settings - Fork 33
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 short circuiting logic when doing exact matches #32
fix short circuiting logic when doing exact matches #32
Conversation
I think while you do have found a bug here the fix is not quite right. The condition should be So you only need to change |
@pascalkuthe yes, but then you're recalculating the score every time there's a match, which you don't need to. The latest commit only computes it once at the end, when the best bonus is known. |
ah I see now, I didn't fully get that on first scan. I would have relied on the compiler to optimize that. That mostly looks like a textbook case for LICM but I guess why not. |
@pascalkuthe I've fixed the other functions, added a couple of tests and updated the CHANGELOG.
I've benchmarked recomputing the score on every match vs once at the end, and I think you're right. There's no difference. |
Is there something left to do here? |
sorry for the delay this got a bit lost in my endless stream of notificatons |
I noticed you stop looking for matches as soon as
score >= self.config.bonus_boundary_white
. I believe this is a typo sincescore
is always>= 16
andbonus_boundary_white
is always<= 10
, which means you always return the score (and position) of the first match.This is done in a bunch of place in the
exact
module. I've only fixed the first one because I wanted to get some feedback before fixing the others. If it LGTY I can go ahead.