-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
allow refs in our constant folder #6144
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
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.
Even though this is a nice improvement in the constant folder, and catches some false positives in the float_cmp
lint, assert_eq
is still linted when it shouldn't.
This example:
const ZERO: f32 = 0.0;
fn main() {
let offset = ZERO;
assert_eq!(offset, ZERO);
}
expands to:
// ...
const ZERO: f32 = 0.0;
fn main() {
let offset = ZERO;
{
match (&offset, &ZERO) {
(left_val, right_val) => {
if !(*left_val == *right_val) {
// panicking code
}
}
}
};
}
So to fix this the constant folder should be taught about the intermediate bindings of the match
.
Would you be willing to add this enhancement in this PR so we can close that issue? We can also land this as is for the moment, as I said it's already an improvement.
I'll add deref, but match is a lot more complex; this will require introducing binding scope. I'd like to leave this to a follow-up PR. |
@ebroto r? |
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'll add deref, but match is a lot more complex; this will require introducing binding scope. I'd like to leave this to a follow-up PR.
Seems fair. Thanks for the enhancement!
@bors r+ |
📌 Commit 26e4de9 has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
This helps with #3804 (but won't completely fix it).
changelog: none