Skip to content
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

implement Hash for Atom #474

Open
dvic opened this issue Jul 7, 2022 · 3 comments
Open

implement Hash for Atom #474

dvic opened this issue Jul 7, 2022 · 3 comments

Comments

@dvic
Copy link
Contributor

dvic commented Jul 7, 2022

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.

@evnu
Copy link
Member

evnu commented Jul 7, 2022

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.

@dvic
Copy link
Contributor Author

dvic commented Jul 7, 2022

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?

@evnu
Copy link
Member

evnu commented Nov 11, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants