-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
feat(stdlib): Add ability to set seed on Hash.hash
#1952
base: main
Are you sure you want to change the base?
Conversation
stdlib/hash.gr
Outdated
*/ | ||
@unsafe | ||
provide let hash = anything => { | ||
h = seed | ||
provide let hash = (anything, seed=seed) => { |
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.
I don't think we want to allow setting a seed on each hash. Instead, I think we want to add a submodule with a Hash record that has the seed attached and then create a singleton at the top-level module that passes it to the submodule hash
. This way people can do Hash.SeededHash.make()
and then Hash.SeededHash.hash(hasher, anything)
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.
I think this would be more elegant if Hash.SeededHash.make()
returned. Function.
Hash.SeededHash.make(seed: Number) -> ((val: a) -> Number)
thoughts?
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.
I don't hate it! Let's get some other feedback before doing the work.
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.
I like Jake's idea; however in that case I'm not sure it needs to be in a submodule if it's just one function; maybe could just make a makeSeeded
function?
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.
I am good with that approach.
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.
In the case of the make function, we'd want seed to be defaulted and use that to construct our global hasher, right?
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.
I dont think we should make it defaulted I think the api should look like.
module Hash
provide let makeSeeded = (seed) => ((value: a) => Number)
let hash = makeSeeded(currentSeedGenCode)
provide let hash = (value) => hash
we could just directly provide hash
but then I think we miss out on the for good graindoc we currently have. Thoughts?
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.
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.
Yeah, our type system can't really support that right now.
I don't love the idea of the top module and the seeded module having such different APIs. I think the SeededHash
module would make more sense if we had functors, e.g. SeededHash.Make()
which would give you just a module with a hash
function on it.
It'd be breaking, but I'd like to see that same hash(hasher, anything)
on both versions, with make
and makeSeeded
functions to make the hasher.
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.
Whoops. Submitted that as a comment.
f2b49c4
to
5c37dc8
Compare
I updated this, due to the issue I ran into above I decided to go with the approach @phated had suggested at first. I also rewrote the hashing function to avoid a mutable global. while this wasn't necessary I think it reduces the surface area for bugs to pop up. |
This should be done in a non-breaking way so I'm updating the title |
Hash.hash
Hash.hash
This pr adds the ability to set the seed of the hashing function.
I think it might be worth pinning the seed when this is used in
Map
andSet
, which would make marshalling the values work between two separate runs.