-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Remove a clone in mir/transform/add_validation #52364
Conversation
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
let local_decls = mir.local_decls.clone(); // FIXME: Find a way to get rid of this clone. | ||
|
||
// Replace the local_decls in mir with a dummy to avoid cloning them | ||
let local_decls = mem::replace(&mut mir.local_decls, IndexVec::new()); |
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.
Wait, why was the clone necessary? There are nowadays methods to get mutable references to both local_decls
and basic_blocks
at the same time.
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.
Maybe there were no such methods when I wrote this code last year?
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.
That aside, is it really okay to just empty the local_decls
?
EDIT: Oh, they are returned later. That should be commented more clearly. And somehow it should make sure that it does not return early. Also, is there a problem in case of a panic?
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 if it's okay, we should try to avoid use methods that just give access, rather than juggling swaps.
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 assume you meant to say "try to use methods"?
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.
Indeed, heh.
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.
Good to know; I'll have a stab at it using those methods.
cc @RalfJung |
r? @RalfJung |
Looks good, thanks! @bors r+ (My first r+... let's hope this works :D ) |
📌 Commit d5219a7 has been approved by |
⌛ Testing commit d5219a7 with merge cb321fcc6a53a5f058086cb8d3f26b05b294810b... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
Looks like a spurious network failure. @bors retry |
⌛ Testing commit d5219a7 with merge 530e50fede072fe68a72642aa5077b1e33e420a7... |
💔 Test failed - status-travis |
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
Your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem. Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@TimNN the bot showed an empty log. |
Looks spurious again. @bors retry |
⌛ Testing commit d5219a7 with merge 5c557c6d82679b49494fde1875105e758f371399... |
💔 Test failed - status-travis |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
1 similar comment
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
@rust-lang/infra Any idea what this error could be about?
(EDIT: Looks like it is #50887) @bors retry |
Remove a clone in mir/transform/add_validation Remove a clone of `mir.local_decls`.
☀️ Test successful - status-appveyor, status-travis |
Tested on commit rust-lang/rust@29ee654. Direct link to PR: <rust-lang/rust#52364> 🎉 book on linux: test-fail → test-pass.
Remove a clone of
mir.local_decls
.