-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Improve Ord docs #129003
base: master
Are you sure you want to change the base?
Improve Ord docs #129003
Conversation
@Voultapher I scanned this a bit. If you want to split it out, the diff that only concerns PartialEq and Eq already has my r+ modulo "Jubilee finds a typo on a second/third pass". The rest will take more time to review. |
@workingjubilee I have a slight preference for keeping it as one PR if that's fine for you. |
@Voultapher Understandable, then! I just wanted to extend the option. |
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 got through the non-example text. The large number of whitespace-only changes are part of why this has been a bit slow to review. They are all very distracting, and some of them actually are incorrect. I haven't reviewed the examples yet.
/// Because of different signatures, `Ord` cannot be a simple marker trait like `Eq`. So it can't be | ||
/// `derive`d automatically when `PartialOrd` is implemented. The recommended best practice for a | ||
/// type that manually implements `Ord` is to implement the equality comparison logic in `PartialEq` | ||
/// and implement the ordering comparison logic in `Ord`. From there one should implement | ||
/// `PartialOrd` as `Some(self.cmp(other))`. |
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.
This seems redundant with the paragraph two paragraphs ago (lines 784~786)? Written twice?
Same "start with the relevance" ask: what we really want to tell people is they should derive PartialEq, PartialOrd, and Ord, xor manually implement all of them, right? Because otherwise they're at risk of being incorrect.
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 wrote those two paragraphs somewhat disjointed in time, and indeed there is substantial overlap, but the intent is a different one in each. The first tries to communicate the "stacked" nature of the traits, and the second tries to communicate that "no it's not you, it's us, implementing them correctly and idiomatically is rather inconsistent at first glance.". I remember stumbling a couple of times over the fact that of the 3 Options that exist:
A) Implement with logic
B) Derive
C) Implement in terms of other
The correct and idiomatic way to implement the four traits is
- PartialEq: A
- Eq: B
- PartialOrd: C
- Ord: A
Despite sounding very similar and having the similar concept of extending the promises made by the partial one, we expect users to follow this at first glance buckwild inconsistency. IMO something like ABAB would much easier to teach and naturally discover, but that comes with its own issues around unwraping and communicating infallibility. What I'm getting at is, I'd like the docs to contain a section that acknowledges the inconsistency.
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 don't mind repeating things slightly or trying to explain things are inconsistent (though, we also are entitled to pause and say something like "Implementing these traits is hazard-prone, you want to do one of these things:"), it just seemed more disjointed than necessary.
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's more disjointed than I like as well, do you have an idea how we could improve it?
I will probably not be able to review the examples without these being fixed, since I will be trying to relate the examples back to prior text. |
- Makes wording more clear and re-structures some sections that can be overwhelming for some not already in the know. - Adds examples of how *not* to implement Ord, inspired by various anti-patterns found in real world code.
In hindsight I should have split the whitespace changes out as a separate commit, sorry. |
9155758
to
386e04e
Compare
I've rebased the branch and added the requested changes as a new commit. |
/// - reflexive: `a == a`; | ||
/// - symmetric: `a == b` implies `b == a` (required by `PartialEq` as well); and | ||
/// - transitive: `a == b` and `b == c` implies `a == c` (required by `PartialEq` as well). | ||
/// - symmetric: `a == b` implies `b == a` |
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 guess also a != b
implies b != a
?
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.
Kinda, !=
in Rust can mean not equal or incomparable, for example f64::NAN != f64::NAN
. I checked with @orlp and he suggests that we express it in terms of a != b
must be equivalent to !(a == b)
.
@workingjubilee I think the PR is waiting on review |
It is, I was working yesterday mostly on making sure backtraces are handled correctly on Android (and a lot of other platforms tbh). |
No worries, please take your time. I mentioned it because the issue has the label |
Oh. |
Many of the wording changes are inspired directly by my personal experience of being confused by the
Ord
docs and seeing other people get it wrong as well, especially lately having looked at a number ofOrd
implementations as part of #128899.Created with help by @orlp.
r? @workingjubilee