helix-editor / nucleo

A fast and convenient fuzzy matcher library for rust
Mozilla Public License 2.0
899 stars 28 forks source link

fix short circuiting logic when doing exact matches #32

Closed noib3 closed 11 months ago

noib3 commented 1 year ago

I noticed you stop looking for matches as soon as score >= self.config.bonus_boundary_white. I believe this is a typo since score is always >= 16 and bonus_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.

pascalkuthe commented 1 year ago

I think while you do have found a bug here the fix is not quite right.

The condition should be bonus >= self.config.bonus_boundary_white (which is the maximum possible bonus and in turn the maximum possible score for a single char match).

So you only need to change if score >= self.config.bonus_boundary_white { to if bonus >= self.config.bonus_boundary_white { (which is indeed a typeo)

noib3 commented 1 year ago

@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.

pascalkuthe commented 1 year ago

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.

noib3 commented 1 year ago

@pascalkuthe I've fixed the other functions, added a couple of tests and updated the CHANGELOG.

That mostly looks like a textbook case for LICM but I guess why not.

I've benchmarked recomputing the score on every match vs once at the end, and I think you're right. There's no difference. The latest commit does the latter but I can change it to how it was originally if you prefer that. Edit: now doing the former for consistency and ease of review.

noib3 commented 12 months ago

Is there something left to do here?

pascalkuthe commented 11 months ago

sorry for the delay this got a bit lost in my endless stream of notificatons