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 Event, PollOpt, Ready #663

Closed
dtolnay opened this Issue Aug 18, 2017 · 10 comments

Comments

Projects
None yet
4 participants
@dtolnay
Copy link

dtolnay commented Aug 18, 2017

Per the API guidelines' recommendation to eagerly implement common traits.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Aug 18, 2017

TBH, I don't know if this makes sense. These values are structured to permit internal bits to be set. As such, equality / hash are hard to cleanly define as they could look to be the same but internally be different.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Aug 18, 2017

Not to mention, in not sure when it would make sense to hash these values.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Aug 18, 2017

@dtolnay

This comment has been minimized.

Copy link
Author

dtolnay commented Aug 18, 2017

If values can externally look the same but internally be different, that indeed rules out Hash.

@alexcrichton

This comment has been minimized.

Copy link
Contributor

alexcrichton commented Aug 18, 2017

@carllerche oh I thought the internal bits were just platform-specific bits, but are there other use cases as well? That is, given any instance of Ready I thought you'd be able to construct and witness what all bits were set.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Aug 18, 2017

@alexcrichton Mio is permitted to use internal Ready bits as it sees fit. I believe that we specifically prevent construction of Ready to / from usize for this. I know for a fact I have at one point used misc read bits to manage custom ready queue state.

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Aug 18, 2017

Looks like I stopped, that said I would want to reserve the ability. I don't think it makes sense to impl Hash.

@alexcrichton

This comment has been minimized.

Copy link
Contributor

alexcrichton commented Aug 18, 2017

Ah ok, makse sense to me! Do you think it'd be bad, though, to implement Eq/Hash for "relevant bits", e.g. those you can otherwise read elsewhere

@carllerche

This comment has been minimized.

Copy link
Owner

carllerche commented Sep 1, 2017

I'm unsure, but I would lean towards being conservative and not have impls because

  • I am unsure you really want to ever do eq / hash of those types
  • There would be a risk of introducing bugs in the internals due to accidentally using those Eq / Hash impls (if the internals expect the private bits to be factored in).

That said, my opinion isn't strong, so if people still think it is worth having these impls, then I am OK w/ it.

@tobz

This comment has been minimized.

Copy link
Collaborator

tobz commented Nov 30, 2018

Gonna close this as it's been sitting for a while with nobody that seems willing to carry it through.

@tobz tobz closed this Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.