-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
MIR drive-by cleanups #111501
MIR drive-by cleanups #111501
Changes from all commits
5ae51d6
03d5f9b
f542778
140cdcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,10 +6,8 @@ | |
|
||
fn main() { | ||
let y = &5; | ||
let x: ! = unsafe { | ||
*(y as *const _ as *const !) //~ ERROR: entering unreachable code | ||
}; | ||
f(x) | ||
let x: ! = unsafe { *(y as *const _ as *const !) }; | ||
f(x) //~ ERROR: entering unreachable code | ||
} | ||
Comment on lines
7
to
11
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is surprising. How does the MIR change here? The previous MIR didn't even contain an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The change is essentially
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it looks like the difference is already present after building MIR: --- ./nightly_mir_dump/t.f.001-000.built.after.mir 2023-05-15 14:25:01.537195340 +0000
+++ ./b_mir_dump/t.f.001-000.built.after.mir 2023-05-15 14:25:07.469143513 +0000
@@ -3,23 +3,9 @@
fn f(_1: !) -> ! {
debug x => _1; // in scope 0 at ./t.rs:9:6: 9:7
let mut _0: !; // return place in scope 0 at ./t.rs:9:15: 9:16
- let mut _2: !; // in scope 0 at ./t.rs:9:17: 11:2
- let mut _3: !; // in scope 0 at ./t.rs:10:5: 10:6
bb0: {
- StorageLive(_2); // scope 0 at ./t.rs:9:17: 11:2
- StorageLive(_3); // scope 0 at ./t.rs:10:5: 10:6
- _3 = _1; // scope 0 at ./t.rs:10:5: 10:6
- unreachable; // scope 0 at ./t.rs:10:5: 10:6
- }
-
- bb1: {
- StorageDead(_3); // scope 0 at ./t.rs:10:5: 10:6
- unreachable; // scope 0 at ./t.rs:9:17: 11:2
- }
-
- bb2: {
- StorageDead(_2); // scope 0 at ./t.rs:11:1: 11:2
+ _0 = _1; // scope 0 at ./t.rs:10:5: 10:6
return; // scope 0 at ./t.rs:11:2: 11:2
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand why There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason is "existence of identity coercions makes my other work harder" 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's somewhat surprising but well, I am not familiar with those parts of the compiler at all.^^ If this is an invariant we want to enforce, should it be checked somewhere so that we notice when identity coercions come back? Still, @rust-lang/types should probably take a look. |
||
|
||
fn f(x: !) -> ! { | ||
|
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 you put this in a different file? This one's mostly for type definitions
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.
There are similar functions in this file though, like
MirPhase::name
...Not sure where to best put this
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.
Hmm, ok. We should probably move those out too, don't worry about it for this PR though
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.
The mir::terminator file looks like a proper place.