-
Notifications
You must be signed in to change notification settings - Fork 88
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
Hashable type class #189
Hashable type class #189
Conversation
This looks like a great start! I'm assuming you didn't just come up with these hashing functions and took them from somewhere else? (The Haskell implementation? Some paper?) I think it would be good to have a few comments referencing where these hashing functions are coming from, to have some form of rationale for them. |
The one for Number is used by immutable.js, among others. I can add comments. It'd be even better if someone did some actual research on the matter. We are in the fortunate position that Eq is homogeneous and there is no subtyping, so changing the algorithm is not a breaking change and can be done for any type individually. |
and fix the record hash function in the process
This now uses the indexed newtype as discussed in #188. |
src/Data/Hashable.purs
Outdated
-- | are never equal. The reverse is not necessarily true. | ||
-- | | ||
-- | Hash values produced by `hash` must not be relied upon to be | ||
-- | stable accross multiple executions of a program and should not be |
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.
Typo: accross
-> across
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.
Thanks!
Hash values of 0 are not uncommon, e.g., 0, false, Nothing, {}, ... We do not want to hash [true], [false, true], [false, false, true], ... to the same value.
Thanks to @rightfold for pointing out the array hash problem. |
@fehrenbach Should this PR be closed in favor of #198? |
Yeah, probably. I should get back to this at some point... |
See #188.