-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add (random) seed api #22
Conversation
Hi, is there any plan for this pr to be merged? |
2132999
to
f990ea2
Compare
Add `FxHasher::with_seed`.
f990ea2
to
1deac0d
Compare
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.
Can you add some tests for this?
One useful test would be to create two FxRandomState
and ensure their seeds aren't the same.
Another possible test would be creating two FxRandomState
on two different threads and ensuring their seeds aren't the same (this is not guaranteed to always pass, but should in practice always pass).
Another useful test would be for FxHasher::with_seed
to use it with two different seeds (and the default) and ensure that hashing some integer with it produces different results in all cases.
86fefef
to
b9ca726
Compare
@Nilstrieb are the tests I added match what you asked for? I also added a |
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.
r=me after style nit (also on the re-exports)
extern crate rand; | ||
|
||
#[cfg(feature = "rand")] | ||
mod random_state; |
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.
Can we add a newline between the modules to make it more obvious that only the former is cfged?
This commit adds an optional dependency on `rand` (behind a `rand`) feature that allows seeding `FxHasher` with random seeds. This is done via `FxRandomState` that implemented similar to `std::collections::hash_map::RandomState`. `FxHashMapRand` and `FxHashSetRand` are also introduced as type aliases to `HashMap` and `HashSet` with `S = FxRandomState`.
b9ca726
to
71de84e
Compare
Uuuuhhhhh is the master branch not protected? D: |
I think it is, but I approved your PR, and GitHub does not dismiss approvals after pushing |
@Nilstrieb I merged the PR before CI passed (or at least it looked like I did, maybe it happened at the same time?). also fyi github has an option to dismiss approvals after pushing |
interesting.. i wouldn't wanna dismiss approvals, that's just annoying, but if it's not gated on CI that would be a bit bad. not that it matters too much |
tl;dr: this PR adds the following APIs:
This adds ways to create
FxHasher
with a custom or random seed.Resolves #14
Resolves #15
Helps with #17, though different default seed may still be a good idea.