rustler icon indicating copy to clipboard operation
rustler copied to clipboard

implement Hash for Atom

Open dvic opened this issue 1 year ago • 3 comments

Can we implement std::hash::Hash for Atom like this:

impl std::hash::Hash for Atom {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        state.write_usize(self.term)
    }
}

If so, I can open up a PR.

dvic avatar Jul 07 '22 11:07 dvic

Note that the same atom (eg hello) could be represented by a different number for different invocations of the VM. Then, the hash for the same atom would also be different. I don't know if this hurts, but it is probably something to think about.

evnu avatar Jul 07 '22 16:07 evnu

Yes and also I now see there is Term::hash_internal. And I also see that there is already an implementation for Term:

impl<'a> Hash for Term<'a> {
    fn hash<H: Hasher>(&self, state: &mut H) {
        // As far as I can see, there is really no way
        // to get a seed from the hasher. This is definitely
        // not optimal, but it's the best we can do for now.
        state.write_u32(self.hash_internal(0));
    }
}

So seems like a non-breaking change to add it for the Atom type as well, I guess?

dvic avatar Jul 07 '22 16:07 dvic

So seems like a non-breaking change to add it for the Atom type as well, I guess?

Yes, I think that makes sense. @rusterlium/core opinions? I kinda worry that user's start to write the hash somewhere and assume that it will not change across VM instances.

evnu avatar Nov 11 '22 10:11 evnu