Better docs for PartialEq (includes macro rename)#156144
Conversation
|
r? @nia-e rustbot has assigned @nia-e. Use Why was this reviewer chosen?The reviewer was selected based on:
|
| fn eq(&self, other: &Rhs) -> bool; | ||
|
|
||
| /// Tests for `!=`. The default implementation is almost always sufficient, | ||
| /// and should not be overridden without very good reason. |
There was a problem hiding this comment.
This warning should probably remain in some form
There was a problem hiding this comment.
I've now tried to reword to mention the few cases where it could make sense to override (line 269).
| /// Implements `PartialEq` for primitive types. | ||
| /// | ||
| /// Primitive types have a compiler-defined primitive implementation of `==` and `!=`. | ||
| /// This implements the `PartialEq` trait is terms of those primitive implementations. |
There was a problem hiding this comment.
typo: "trait [in] terms of"
|
Looks mostly fine, just a few comments. Thanks! |
a52171b to
d3db8b5
Compare
d3db8b5 to
f25a255
Compare
|
@rustbot ready |
| /// The default implementation of the inequality operator simply calls | ||
| /// the implementation of the equality operator and negates the result. | ||
| /// | ||
| /// Override the default when forwarding to another PartialEq implementation, |
There was a problem hiding this comment.
| /// Override the default when forwarding to another PartialEq implementation, | |
| /// This default shouldn't be overriden without very good reason, such as when |
There was a problem hiding this comment.
I'm happy to go with this, but I'm not convinced we really need this strong warning. In fact I'm not convinced we really need a warning at all here, as nothing really bad can happen when overriding the default (except for getting it wrong accidentally). In fact we seem to prefer to override the default for forwarding implementations for only a small win.
@fee1-dead I would appreciate your perspective on this.
| /// the implementation of the equality operator and negates the result. | ||
| /// | ||
| /// Override the default when forwarding to another PartialEq implementation, | ||
| /// and maybe when using FFI or inline assembly for its implementation. |
There was a problem hiding this comment.
| /// and maybe when using FFI or inline assembly for its implementation. | |
| /// forwarding to another implementation. |
|
r=me if the changes seem reasonable, thanks! |
No description provided.