-
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
update ancient note #113618
update ancient note #113618
Conversation
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
/// [`Eq`] and [`Hash`]. We must also derive [`PartialEq`], this will in the | ||
/// future be implied by [`Eq`]. | ||
/// [`Eq`] and [`Hash`]. We must also derive [`PartialEq`], | ||
/// which is implied by [`Eq`]. | ||
/// |
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 think this change is correct. the note is saying "we have to derive PartialEq because Eq without PartialEq will error, and at some point we want to change derive(Eq) to imply derive(PartialEq)". that second half still isn't the case today — it's probably true that it's never going to happen, but i would expect the new comment to look a little different if that's what you meant.
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 suspected that could be the case, but got confused by:
This property cannot be checked by the compiler, and therefore Eq implies PartialEq, and has no extra methods.
from https://doc.rust-lang.org/std/cmp/trait.Eq.html
I guess implies means something else in that example?
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.
yes, unfortunately. the one here is using it to describe a behavior of the compiler and the one on the trait page is using it to describe a property of the language (that your code won't compile if you have Eq without PartialEq).
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.
please see if latest wording is acceptable
@bors r+ rollup |
update ancient note
update ancient note
update ancient note
…iaskrgr Rollup of 5 pull requests Successful merges: - rust-lang#112525 (Adjustments for RustyHermit) - rust-lang#112729 (Add machine-applicable suggestion for `unused_qualifications` lint) - rust-lang#113618 (update ancient note) - rust-lang#113640 (Make `nodejs` control the default for RustdocJs tests instead of a hard-off switch) - rust-lang#113668 (Correct `the` -> `there` typo in items.md) r? `@ghost` `@rustbot` modify labels: rollup
No description provided.