-
Notifications
You must be signed in to change notification settings - Fork 13.4k
fix(#141141): When expanding PartialEq
, check equality of scalar types first.
#141724
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
Conversation
This comment has been minimized.
This comment has been minimized.
Changes to the code generated for builtin derived traits. cc @nnethercote |
self.b2 == other.b2 && self.b4 == other.b4 && self.b5 == other.b5 && | ||
self.b6 == other.b6 && self.b7 == other.b7 && | ||
self.b8 == other.b8 && self.b10 == other.b10 && | ||
(self.b1 == other.b1 && self.b3 == other.b3 && self._b == other._b |
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.
Can these parentheses be avoided? They're not 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.
Parentheses are generated because the comparison expressions for scalar and compound fields are grouped and then combined in a single pass. As far as I understand, removing these parentheses would require iterating over the fields twice: once to collect all scalar-typed fields, and a second time to add the comparisons for compound-typed fields. Would making two passes over the fields be acceptable for this purpose?
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.
Two passes sounds fine, the number of fields is always small so the performance effect should be negligible.
(ReorderEnum::G(__self_0, __self_1, __self_2), | ||
ReorderEnum::G(__arg1_0, __arg1_1, __arg1_2)) => | ||
__self_1 == __arg1_1 && | ||
(__self_0 == __arg1_0 && __self_2 == __arg1_2), |
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.
Can these parens be avoided?
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.
Looking good, just a few nits to fix. Thanks.
@rustbot author
compiler/rustc_ast/src/ast.rs
Outdated
@@ -2452,6 +2452,32 @@ impl TyKind { | |||
None | |||
} | |||
} | |||
|
|||
pub fn maybe_scalar(&self) -> bool { | |||
let Some(ty_kind) = self.is_simple_path() else { |
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.
ty_sym
would be a better name than ty_kind
. I was momentarily confused because I thought this would have TyKind
.
@@ -2452,6 +2452,32 @@ impl TyKind { | |||
None | |||
} | |||
} | |||
|
|||
pub fn maybe_scalar(&self) -> bool { |
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.
A brief comment explaining the maybe_
prefix would be good, i.e. that it's not perfectly accurate because of typedefs.
#[inline] | ||
fn partial_cmp(&self, other: &Reorder) | ||
-> ::core::option::Option<::core::cmp::Ordering> { | ||
match ::core::cmp::PartialOrd::partial_cmp(&self.b1, &other.b1) { |
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.
Scalars are being checked first for eq
. But partial_cmp
is still using the old field ordering. Is this intentional?
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.
And likewise for the enum case below.
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.
Optimizing partial_cmp
while keeping its guarantees would require a separate issue. This PR focuses on the PartialEq::eq
.
|
||
/// Generates the equality expression for a struct or enum variant when deriving `PartialEq`. | ||
/// | ||
/// This function generates an expression that checks if all fields of a struct or enum variant are equal. |
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.
A few lines exceed 100 chars, please reflow the comments.
Reminder, once the PR becomes ready for a review, use |
…he order of declaration.
@rustbot ready |
Thank you! @bors r+ |
Rollup of 8 pull requests Successful merges: - #141724 (fix(#141141): When expanding `PartialEq`, check equality of scalar types first.) - #141833 (`tests/ui`: A New Order [2/N]) - #141861 (Switch `x86_64-msvc-{1,2}` back to Windows Server 2025 images) - #141914 (redesign stage 0 std follow-ups) - #141918 (Deconstruct values in the THIR visitor) - #141923 (Update books) - #141931 (Deconstruct values in the THIR visitor) - #141956 (Remove two trait methods from cg_ssa) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of #141724 - Sol-Ell:issue-141141-fix, r=nnethercote fix(#141141): When expanding `PartialEq`, check equality of scalar types first. Fixes #141141. Now, `cs_eq` function of `partial_eq.rs` compares [scalar types](https://doc.rust-lang.org/rust-by-example/primitives.html#scalar-types) first. - Add `is_scalar` field to `FieldInfo`. - Add `is_scalar` method to `TyKind`. - Pass `FieldInfo` via `CsFold::Combine` and refactor code relying on it. - Implement `TryFrom<&str>` and `TryFrom<Symbol>` for FloatTy. - Implement `TryFrom<&str>` and `TryFrom<Symbol>` for IntTy. - Implement `TryFrom<&str>` and `TryFrom<Symbol>` for UintTy.
Fixes #141141.
Now,
cs_eq
function ofpartial_eq.rs
compares scalar types first.is_scalar
field toFieldInfo
.is_scalar
method toTyKind
.FieldInfo
viaCsFold::Combine
and refactor code relying on it.TryFrom<&str>
andTryFrom<Symbol>
for FloatTy.TryFrom<&str>
andTryFrom<Symbol>
for IntTy.TryFrom<&str>
andTryFrom<Symbol>
for UintTy.