-
Notifications
You must be signed in to change notification settings - Fork 339
grouping_map: Allow to create GroupingMap with a custom BuildHasher
#1057
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
base: master
Are you sure you want to change the base?
grouping_map: Allow to create GroupingMap with a custom BuildHasher
#1057
Conversation
Currently, `GroupingMap` can create a `HashMap` with the default `RandomState`. For some kind of keys, using different `BuildHasher` implementation can result in better performance. Add a possibility to create a `GroupingMap` with a custom hasher builder, which then produces a `HashMap` with it.
|
Thanks for this. Closing this for now, because - if we do this - we won't restrict ourselves to custom |
|
We've been blocked for ages on the problem of abstracting over container type. Abstracting over hasher is a much simpler problem and, with type defaults, perhaps even a non-breaking change. I think we should consider this PR. |
|
Re-opening as per @jswrenn's comment. |
phimuemue
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, this looks all reasonable to me.
The only nit I have is that I'd ideally like to have less builder functions. Maybe all ::new-functions could call the respective ::with_hasher-functions, and we could get rid of grouping_map::new?
@jswrenn I'm no expert in the not-a-breaking-change business. I guess this one could be ok, because we "only" add a S: BuildHasher here and there, but existing callers may be unaffected. Could you offer a second look at this, and if you, too, say, this is fine, I'd merge it.
| { | ||
| GroupingMap { iter } | ||
| let hash_builder = RandomState::new(); | ||
| GroupingMap { iter, hash_builder } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a nit, but please forward this to with_hasher(iter, RandomState::new()).
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1057 +/- ##
==========================================
- Coverage 94.38% 93.35% -1.03%
==========================================
Files 48 50 +2
Lines 6665 6355 -310
==========================================
- Hits 6291 5933 -358
- Misses 374 422 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Currently,
GroupingMapcan create aHashMapwith the defaultRandomState.For some kind of keys, using different
BuildHasherimplementation can result in better performance. Add a possibility to create aGroupingMapwith a custom hasher builder, which then produces aHashMapwith it.