-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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 ops::Try for Ordering #76041
impl ops::Try for Ordering #76041
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @joshtriplett (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
All trait impls are insta-stable, regardless of what attribute is put on them, sadly. If this were able to go in unstable I'd be all for it, but the |
2df8bfd
to
c9f756c
Compare
I agree it's awkward to use It's unfortunate that implementations ignore stability attributes, especially since the r? @scottmcm |
bd03146
to
d65775e
Compare
Sorry, I should have clarified better -- we found out with So my personal inclination is that -- while I think this is a good impl to have conceptually and implements the semantics I would expect -- it shouldn't happen until either:
But we'll see what T-libs has to say about things. |
marked it as s-waiting-on-team because this is awaiting feedback by @rust-lang/libs . The discussion can be found here |
Doesn't seemed straightforward that not equal |
I agree it's not obvious from the
struct Test(bool, u8, &'static str);
impl Ord for Test {
fn cmp(&self, other: &Self) -> std::cmp::Ordering {
self.0.cmp(&other.0)?;
self.1.cmp(&other.1)?;
self.2.cmp(&other.2)
}
}
impl PartialEq for Test {
fn eq(&self, other: &Self) -> bool {
self.0.eq(&other.0)?;
self.1.eq(&other.1)?;
self.2.eq(&other.2)
}
} I'm not opposed to adding |
d65775e
to
4558600
Compare
We discussed this in the libs team meeting, and agreed that—at least right now—this usage of the |
I wasn't at the libs meeting, but I'd like to echo my strong agreement with that position. I feel very uneasy about adding |
Last time I used to be thinking that |
} | ||
|
||
// This Ord implementation should behave the same as #[derive(Ord)], | ||
// but uses the Try operator on Ordering values |
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.
// but uses the Try operator on Ordering values | |
// but uses the `?` operator on Ordering values |
For clarity
} | ||
} | ||
|
||
// Implement Iterator::cmp() using the Try operator |
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.
// Implement Iterator::cmp() using the Try operator | |
// Implement Iterator::cmp() using the `?` operator |
For clarity.
Thanks for the PR, @calebsander! Given this libs team feedback I'm going to close this PR. It sounds like it might be worth keeping in mind in other try-related conversations, but that we probably need more experience with other things before we can consider it. |
For anyone following along, I've posted an RFC that would resolve my this would allow Note, however, that said RFC takes no position on whether implementations like this on on |
Implements
Try
forOrdering
, mirroring the behavior ofOrdering::then()
. (Applying?
toLess
orGreater
causes it to bereturn
ed immediately, whileEqual?
results in()
.) This is useful for implementing lexicographic orderings, where comparisons are chained until the first one that returns non-Equal
. See the examples below.I've currently gated this behind the
try_trait
feature, but maybe a separate feature should be used instead? I was surprised that the test passed without#![feature(try_trait)]
; is this expected behavior?Example 1: comparing structs as
#[derive(Ord)]
would doExample 2: comparing iterators as
Iterator::cmp()
would do