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 instances fix #111
Hashable instances fix #111
Conversation
Doesn't pass as instance Hashable (HashMap k v) violates (x == y) ==> hashWithSalt salt x == hashWithSalt salt y property.
94c4a3c
to
0a13436
Compare
Resolves #78 |
pHashable xs is salt = | ||
x == y ==> hashWithSalt salt x === hashWithSalt salt y | ||
where | ||
ys = L.map snd . L.sort . L.zip (is ++ [L.maximum (0:is) + 1..]) $ xs |
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.
Could you add a comment what this line is trying to achieve (e.g. would it be better to randomly shuffle ys
)?
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 actually to randomly shuffle xs
into ys
, I'll give that shuffle stuff a name and a comment
Clarified the test. I don't know better way to have a list and randomly shuffled version of it for the property. |
@tibbe Is there something still needing a fix or an improvement? |
go :: Int -> [HashMap k v] -> Int | ||
go s [] = s | ||
go s (Leaf _ l : tl) = s `hashLeafWithSalt` l `go` tl | ||
-- For collisions we hashmix hash value, and then array of values' hashes sorted |
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.
Please keep line length <= 80 chars. Here and elsewhere.
05a2554
to
69151b7
Compare
All lines are under 80 characters now. |
Should be ready to review.