-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
impl Hash for ! #51404
impl Hash for ! #51404
Conversation
r? @KodrAus (rust_highfive has picked a reviewer for you, use r? to override) |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/libcore/hash/mod.rs
Outdated
#[stable(feature = "never_hash", since = "1.29.0")] | ||
impl Hash for ! { | ||
fn hash<H: Hasher>(&self, _: &mut H) { | ||
unreachable!() |
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.
It doesn't seem that important, but using *self
instead of unreachable!()
is more consistent with how other impls for !
are defined.
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.
@varkor I was under the impression that impls for !
which return a value of another type generally do *self
, but ones which return unit do unreachable!()
. Although this seems pretty arbitrary and I may have just misremembered what I saw of existing impls.
I can change it to *self
if that seems more reasonable.
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.
All the implementations I can see in libstd use *self
, though there are a couple in rustc that use other methods. I don't think it's hugely important though — it's fairly obvious what's going on with either method.
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 like *self
just so that it's trivial to translate -- no need to emit a bunch of panic call HIR+MIR+IR only for LLVM to figure out that it's unneeded again and remove it all.
Rebased and replaced |
Sorry for taking a while to get to this! It looks good to me. @bors r+ |
📌 Commit 570590f has been approved by |
impl Hash for ! This was missing in some generic code I was writing and I figured that it would be worthwhile to add this. Blanket-requiring these traits to allow usage of errors in `HashSet`s and `HashMap`s isn't too unreasonable of a use case, and a prerequisite for allowing `!` as an error in such situations is this impl.
☀️ Test successful - status-appveyor, status-travis |
Implement Hash for Infallible https://www.reddit.com/r/rust/comments/fmllgx/never_crate_stable_alternative_to/ lists not implementing `Hash` as a reason for the `never` crate. I see no reason not to implement `Hash` for `Infallible`, so might as well do it. No changes necessary for `!`, because `!` already implements `Hash` (see rust-lang#51404).
Implement Hash for Infallible https://www.reddit.com/r/rust/comments/fmllgx/never_crate_stable_alternative_to/ lists not implementing `Hash` as a reason for the `never` crate. I see no reason not to implement `Hash` for `Infallible`, so might as well do it. No changes necessary for `!`, because `!` already implements `Hash` (see rust-lang#51404).
Implement Hash for Infallible https://www.reddit.com/r/rust/comments/fmllgx/never_crate_stable_alternative_to/ lists not implementing `Hash` as a reason for the `never` crate. I see no reason not to implement `Hash` for `Infallible`, so might as well do it. No changes necessary for `!`, because `!` already implements `Hash` (see rust-lang#51404).
This was missing in some generic code I was writing and I figured that it would be worthwhile to add this. Blanket-requiring these traits to allow usage of errors in
HashSet
s andHashMap
s isn't too unreasonable of a use case, and a prerequisite for allowing!
as an error in such situations is this impl.