-
Notifications
You must be signed in to change notification settings - Fork 13k
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 Ord for mem::Discriminant #51561
Comments
This was brought up in #24263 (comment) and there were no objections, but it was determined not to be a blocker for stabilization. |
I'm opposed to this since it means that any reordering of enum variants is now a runtime breaking change. Right now reordering is fine as long as the enum author didn't derive (Partial)Ord; making variant order always observable to anyone takes that out of the type's author's control. |
That's a good point that I can't but agree with.
|
I don't think (And I kinda want a lint about |
It seems like if you would bother implementing the marker trait then you might as well provide a PartialOrd impl or a |
In derive-where we wanted to use this to properly implement impl Ord for Test {
fn cmp(&self, other: &Self) -> Ordering {
if mem::discriminant(self) == mem::discriminant(other) {
Ordering::Equal
} else {
match self {
Test::A => Ordering::Less,
Test::B => match other {
Test::A => Ordering::Greater,
_ => Ordering::Less,
},
Test::C => match other {
Test::A | Test::B => Ordering::Greater,
_ => Ordering::Less,
},
Test::D => Ordering::Greater,
}
}
}
} Versus: impl Ord for Test {
fn cmp(&self, other: &Self) -> Ordering {
let self_disc = mem::discriminant(self);
let other_disc = mem::discriminant(other);
if self_disc == other_disc {
Ordering::Equal
} else {
Ord::cmp(&self_disc, &other_disc)
}
}
} The argument against it because of stability is a bit confusing to me, to quote Rusts standard library
Maybe this was added later? In any case, to efficiently implement |
Sorting a collection by variant can be useful. For example, to implement
PartialEq
in a simple way.The text was updated successfully, but these errors were encountered: