Runtime customization of byte-frequency table #118
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Ah, in the docs I renamed |
|
Yeah when you make changes, for a small PR like this, just keep everything in one commit. You can force push. |
|
Hmmm can you submit another PR with the default type parameter approach? I think I'd still prefer that route if it can be made to work. Just wanted to look at it and play with it myself. |
sentrip
force-pushed
the
master
branch
2 times, most recently
from
482d61d to
ebf41ed
Compare
|
Yes I will upload it now, and thanks for the tip! |
…oduction of the trait.
|
All the checks seem to pass, all that is left here is the names. My favorites are |
BurntSushi
pushed a commit
that referenced
this pull request
Jul 11, 2023
This makes it possible for the caller to provide their own ranking function for individual bytes. This can potentially speed up searches if one has a better guess than the default for the frequency distribution of bytes in a particular haystack. There is a lot of ceremony here, and it basically boils down to supporting this in no-std no-alloc configurations. I was tempted to just require alloc for this sort of thing and ask for something like `Arc<dyn Fn(u8) -> u8>`, but that would require some ceremony of its own internally to deal with in the no-alloc case. And forcing an allocation for every searcher construction that uses a customer ranker feels like bad juju to me. Another choice would be to just ask for a `fn(u8) -> u8`, but this makes the case of "I analyzed a haystack at runtime to build my ranker" more difficult. Not impossible. But annoying. Yet another choice was to add the trait as in this commit, and then add it as a new type parameter to `FinderBuilder`. I believe this would work, but it requires complicating the public API even more and imposes constraints on the trait (for example, it would want to be `Clone` at least in order to avoid backwards incompatible changes in the `FinderBuilder` API). There's also just generally more ceremony with having to add a type parameter everywhere. Since we only need the ranking function at searcher construction time, we can ask for it at the time of construction and then get rid of it, thus avoiding it infecting everything else. Fixes #117, Closes #118, Closes #119
BurntSushi
pushed a commit
that referenced
this pull request
Jul 11, 2023
This makes it possible for the caller to provide their own ranking function for individual bytes. This can potentially speed up searches if one has a better guess than the default for the frequency distribution of bytes in a particular haystack. There is a lot of ceremony here, and it basically boils down to supporting this in no-std no-alloc configurations. I was tempted to just require alloc for this sort of thing and ask for something like `Arc<dyn Fn(u8) -> u8>`, but that would require some ceremony of its own internally to deal with in the no-alloc case. And forcing an allocation for every searcher construction that uses a customer ranker feels like bad juju to me. Another choice would be to just ask for a `fn(u8) -> u8`, but this makes the case of "I analyzed a haystack at runtime to build my ranker" more difficult. Not impossible. But annoying. Yet another choice was to add the trait as in this commit, and then add it as a new type parameter to `FinderBuilder`. I believe this would work, but it requires complicating the public API even more and imposes constraints on the trait (for example, it would want to be `Clone` at least in order to avoid backwards incompatible changes in the `FinderBuilder` API). There's also just generally more ceremony with having to add a type parameter everywhere. Since we only need the ranking function at searcher construction time, we can ask for it at the time of construction and then get rid of it, thus avoiding it infecting everything else. Fixes #117, Closes #118, Closes #119
BurntSushi
pushed a commit
that referenced
this pull request
Jul 11, 2023
This makes it possible for the caller to provide their own ranking function for individual bytes. This can potentially speed up searches if one has a better guess than the default for the frequency distribution of bytes in a particular haystack. There is a lot of ceremony here, and it basically boils down to supporting this in no-std no-alloc configurations. I was tempted to just require alloc for this sort of thing and ask for something like `Arc<dyn Fn(u8) -> u8>`, but that would require some ceremony of its own internally to deal with in the no-alloc case. And forcing an allocation for every searcher construction that uses a customer ranker feels like bad juju to me. Another choice would be to just ask for a `fn(u8) -> u8`, but this makes the case of "I analyzed a haystack at runtime to build my ranker" more difficult. Not impossible. But annoying. Yet another choice was to add the trait as in this commit, and then add it as a new type parameter to `FinderBuilder`. I believe this would work, but it requires complicating the public API even more and imposes constraints on the trait (for example, it would want to be `Clone` at least in order to avoid backwards incompatible changes in the `FinderBuilder` API). There's also just generally more ceremony with having to add a type parameter everywhere. Since we only need the ranking function at searcher construction time, we can ask for it at the time of construction and then get rid of it, thus avoiding it infecting everything else. Fixes #117, Closes #118, Closes #119
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.