-
Notifications
You must be signed in to change notification settings - Fork 13.4k
[MIR] Get rid of that nasty unit_ty temporary lval #30635
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
Changes from all commits
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 |
---|---|---|
|
@@ -58,7 +58,14 @@ impl<'a,'tcx> Builder<'a,'tcx> { | |
}); | ||
|
||
unpack!(then_block = this.into(destination, then_block, then_expr)); | ||
unpack!(else_block = this.into(destination, else_block, else_expr)); | ||
else_block = if let Some(else_expr) = else_expr { | ||
unpack!(this.into(destination, else_block, else_expr)) | ||
} else { | ||
// Body of the `if` expression without an `else` clause must return `()`, thus | ||
// we implicitly generate a `else {}` if it is not specified. | ||
this.cfg.push_assign_unit(else_block, expr_span, &Lvalue::ReturnPointer); | ||
else_block | ||
}; | ||
|
||
let join_block = this.cfg.start_new_block(); | ||
this.cfg.terminate(then_block, Terminator::Goto { target: join_block }); | ||
|
@@ -157,11 +164,18 @@ impl<'a,'tcx> Builder<'a,'tcx> { | |
} | ||
|
||
// execute the body, branching back to the test | ||
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. we should add at least a comment here explaining that we are (ab)using 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. and that we think this is ok because (a) the type of |
||
let unit_temp = this.unit_temp.clone(); | ||
let body_block_end = unpack!(this.into(&unit_temp, body_block, body)); | ||
// We write body’s “return value” into the destination of loop. This is fine, | ||
// because: | ||
// | ||
// * In Rust both loop expression and its body are required to have `()` | ||
// as the “return value”; | ||
// * The destination will be considered uninitialised (given it was | ||
// uninitialised before the loop) during the first iteration, thus | ||
// disallowing its use inside the body. Alternatively, if it was already | ||
// initialised, the `destination` can only possibly have a value of `()`, | ||
// therefore, “mutating” the destination during iteration is fine. | ||
let body_block_end = unpack!(this.into(destination, body_block, body)); | ||
this.cfg.terminate(body_block_end, Terminator::Goto { target: loop_block }); | ||
|
||
// final point is exit_block | ||
exit_block.unit() | ||
}) | ||
} | ||
|
@@ -206,7 +220,13 @@ impl<'a,'tcx> Builder<'a,'tcx> { | |
this.break_or_continue(expr_span, label, block, |loop_scope| loop_scope.break_block) | ||
} | ||
ExprKind::Return { value } => { | ||
unpack!(block = this.into(&Lvalue::ReturnPointer, block, value)); | ||
block = match value { | ||
Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)), | ||
None => { | ||
this.cfg.push_assign_unit(block, expr_span, &Lvalue::ReturnPointer); | ||
block | ||
} | ||
}; | ||
let extent = this.extent_of_outermost_scope(); | ||
this.exit_scope(expr_span, extent, block, END_BLOCK); | ||
this.cfg.start_new_block().unit() | ||
|
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.
Do we still need the impl of
EvalInto
forOption<ExprRef<'tcx>>
? I suspect not. In any case I think perhaps we ought to remove it, since it's ambiguous what should happen onNone
.