-
Notifications
You must be signed in to change notification settings - Fork 13k
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
some minor code simplifications #122636
some minor code simplifications #122636
Conversation
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to the CTFE / Miri engine cc @rust-lang/miri |
"`validation_in_progress` was already set" | ||
); | ||
let res = f(); | ||
assert!( | ||
self.memory.validation_in_progress.replace(false) == true, |
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 these out quite deliberately, to make it much more clear what is happening: we are setting to true
from false
, and then back to false
from true
. This can be nicely expressed as assert!(val.replace(new) == old)
. Just because the type here is bool
does not mean we should deviate from that pattern, IMO. The code becomes harder to understand with your change.
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.
reverted
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.
Is this some clippy lint or so? We should probably allow
it or add a comment or so, otherwise someone else will inevitably try to do the same again.
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 think it was needless_bool
but so far I have been hesitant on sprinkling alllow
attributes all over the codebase.
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.
In this case I think it would make a lot of sense. When I wrote the code I already half-expected some clippy-driven cleanup to happen, I just didn't know how to best defend against that.
This comment has been minimized.
This comment has been minimized.
@bors r+ rollup |
…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#120640 (Mark UEFI std support as WIP) - rust-lang#121862 (Add release notes for 1.77.0) - rust-lang#122572 (add test for rust-lang#122301 to cover behavior that's on stable) - rust-lang#122578 (Only invoke `decorate` if the diag can eventually be emitted) - rust-lang#122615 (Mention Zalathar for coverage changes) - rust-lang#122636 (some minor code simplifications) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#122636 - matthiaskrgr:compl3, r=compiler-errors some minor code simplifications
…hiaskrgr interpret/memory: explain why we use == on bool This came up in rust-lang#122636.
…hiaskrgr interpret/memory: explain why we use == on bool This came up in rust-lang#122636.
Rollup merge of rust-lang#122654 - RalfJung:interpret-comment, r=matthiaskrgr interpret/memory: explain why we use == on bool This came up in rust-lang#122636.
No description provided.