Address comments

Moves `stmt_expr` into its own module, `expr::stmt`.

src/librustc_mir/build/block.rs

@@ -9,12 +9,9 @@
 // except according to those terms.
 
 use build::{BlockAnd, BlockAndExtension, Builder};
-use build::scope::LoopScope;
 use hair::*;
-use rustc::middle::region::CodeExtent;
 use rustc::mir::repr::*;
 use rustc::hir;
-use syntax::codemap::Span;
 
 impl<'a,'tcx> Builder<'a,'tcx> {
     pub fn ast_block(&mut self,
@@ -82,118 +79,4 @@ impl<'a,'tcx> Builder<'a,'tcx> {
             block.unit()
         })
     }
-
-    pub fn stmt_expr(&mut self, mut block: BasicBlock, expr: Expr<'tcx>) -> BlockAnd<()> {
-        let this = self;
-        let expr_span = expr.span;
-        let scope_id = this.innermost_scope_id();
-        // Handle a number of expressions that don't need a destination at all. This
-        // avoids needing a mountain of temporary `()` variables.
-        match expr.kind {
-            ExprKind::Scope { extent, value } => {
-                let value = this.hir.mirror(value);
-                this.in_scope(extent, block, |this, _| this.stmt_expr(block, value))
-            }
-            ExprKind::Assign { lhs, rhs } => {
-                let lhs = this.hir.mirror(lhs);
-                let scope_id = this.innermost_scope_id();
-                let lhs_span = lhs.span;
-                let lhs_ty = lhs.ty;
-
-                let lhs_needs_drop = this.hir.needs_drop(lhs_ty);
-
-                // Note: we evaluate assignments right-to-left. This
-                // is better for borrowck interaction with overloaded
-                // operators like x[j] = x[i].
-
-                // Generate better code for things that don't need to be
-                // dropped. We need the temporary as_operand generates
-                // so we can clean up the data if evaluating the LHS unwinds,
-                // but if the LHS (and therefore the RHS) doesn't need
-                // unwinding, we just translate directly to an rvalue instead.
-                let rhs = if lhs_needs_drop {
-                    let op = unpack!(block = this.as_operand(block, rhs));
-                    Rvalue::Use(op)
-                } else {
-                    unpack!(block = this.as_rvalue(block, rhs))
-                };
-
-                let lhs = unpack!(block = this.as_lvalue(block, lhs));
-                unpack!(block = this.build_drop(block, lhs_span, lhs.clone(), lhs_ty));
-                this.cfg.push_assign(block, scope_id, expr_span, &lhs, rhs);
-                block.unit()
-            }
-            ExprKind::AssignOp { op, lhs, rhs } => {
-                // FIXME(#28160) there is an interesting semantics
-                // question raised here -- should we "freeze" the
-                // value of the lhs here?  I'm inclined to think not,
-                // since it seems closer to the semantics of the
-                // overloaded version, which takes `&mut self`.  This
-                // only affects weird things like `x += {x += 1; x}`
-                // -- is that equal to `x + (x + 1)` or `2*(x+1)`?
-
-                // As above, RTL.
-                let rhs = unpack!(block = this.as_operand(block, rhs));
-                let lhs = unpack!(block = this.as_lvalue(block, lhs));
-
-                // we don't have to drop prior contents or anything
-                // because AssignOp is only legal for Copy types
-                // (overloaded ops should be desugared into a call).
-                this.cfg.push_assign(block, scope_id, expr_span, &lhs,
-                                     Rvalue::BinaryOp(op,
-                                                      Operand::Consume(lhs.clone()),
-                                                      rhs));
-
-                block.unit()
-            }
-            ExprKind::Continue { label } => {
-                this.break_or_continue(expr_span, label, block,
-                                       |loop_scope| loop_scope.continue_block)
-            }
-            ExprKind::Break { label } => {
-                this.break_or_continue(expr_span, label, block, |loop_scope| {
-                    loop_scope.might_break = true;
-                    loop_scope.break_block
-                })
-            }
-            ExprKind::Return { value } => {
-                block = match value {
-                    Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)),
-                    None => {
-                        this.cfg.push_assign_unit(block, scope_id,
-                                                  expr_span, &Lvalue::ReturnPointer);
-                        block
-                    }
-                };
-                let extent = this.extent_of_return_scope();
-                let return_block = this.return_block();
-                this.exit_scope(expr_span, extent, block, return_block);
-                this.cfg.start_new_block().unit()
-            }
-            _ => {
-                let expr_span = expr.span;
-                let expr_ty = expr.ty;
-                let temp = this.temp(expr.ty.clone());
-                unpack!(block = this.into(&temp, block, expr));
-                unpack!(block = this.build_drop(block, expr_span, temp, expr_ty));
-                block.unit()
-            }
-        }
-    }
-
-    fn break_or_continue<F>(&mut self,
-                            span: Span,
-                            label: Option<CodeExtent>,
-                            block: BasicBlock,
-                            exit_selector: F)
-                            -> BlockAnd<()>
-        where F: FnOnce(&mut LoopScope) -> BasicBlock
-    {
-        let (exit_block, extent) = {
-            let loop_scope = self.find_loop_scope(span, label);
-            (exit_selector(loop_scope), loop_scope.extent)
-        };
-        self.exit_scope(span, extent, block, exit_block);
-        self.cfg.start_new_block().unit()
-    }
 }

src/librustc_mir/build/expr/mod.rs

@@ -75,5 +75,6 @@ mod as_lvalue;
 mod as_rvalue;
 mod as_operand;
 mod as_temp;
-pub mod category;
+mod category;
 mod into;
+mod stmt;

src/librustc_mir/build/expr/stmt.rs

@@ -0,0 +1,135 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+use build::{BlockAnd, BlockAndExtension, Builder};
+use build::scope::LoopScope;
+use hair::*;
+use rustc::middle::region::CodeExtent;
+use rustc::mir::repr::*;
+use syntax::codemap::Span;
+
+impl<'a,'tcx> Builder<'a,'tcx> {
+
+    pub fn stmt_expr(&mut self, mut block: BasicBlock, expr: Expr<'tcx>) -> BlockAnd<()> {
+        let this = self;
+        let expr_span = expr.span;
+        let scope_id = this.innermost_scope_id();
+        // Handle a number of expressions that don't need a destination at all. This
+        // avoids needing a mountain of temporary `()` variables.
+        match expr.kind {
+            ExprKind::Scope { extent, value } => {
+                let value = this.hir.mirror(value);
+                this.in_scope(extent, block, |this, _| this.stmt_expr(block, value))
+            }
+            ExprKind::Assign { lhs, rhs } => {
+                let lhs = this.hir.mirror(lhs);
+                let rhs = this.hir.mirror(rhs);
+                let scope_id = this.innermost_scope_id();
+                let lhs_span = lhs.span;
+
+                let lhs_ty = lhs.ty;
+                let rhs_ty = rhs.ty;
+
+                let lhs_needs_drop = this.hir.needs_drop(lhs_ty);
+                let rhs_needs_drop = this.hir.needs_drop(rhs_ty);
+
+                // Note: we evaluate assignments right-to-left. This
+                // is better for borrowck interaction with overloaded
+                // operators like x[j] = x[i].
+
+                // Generate better code for things that don't need to be
+                // dropped.
+                let rhs = if lhs_needs_drop || rhs_needs_drop {
+                    let op = unpack!(block = this.as_operand(block, rhs));
+                    Rvalue::Use(op)
+                } else {
+                    unpack!(block = this.as_rvalue(block, rhs))
+                };
+
+                let lhs = unpack!(block = this.as_lvalue(block, lhs));
+                unpack!(block = this.build_drop(block, lhs_span, lhs.clone(), lhs_ty));
+                this.cfg.push_assign(block, scope_id, expr_span, &lhs, rhs);
+                block.unit()
+            }
+            ExprKind::AssignOp { op, lhs, rhs } => {
+                // FIXME(#28160) there is an interesting semantics
+                // question raised here -- should we "freeze" the
+                // value of the lhs here?  I'm inclined to think not,
+                // since it seems closer to the semantics of the
+                // overloaded version, which takes `&mut self`.  This
+                // only affects weird things like `x += {x += 1; x}`
+                // -- is that equal to `x + (x + 1)` or `2*(x+1)`?
+
+                // As above, RTL.
+                let rhs = unpack!(block = this.as_operand(block, rhs));
+                let lhs = unpack!(block = this.as_lvalue(block, lhs));
+
+                // we don't have to drop prior contents or anything
+                // because AssignOp is only legal for Copy types
+                // (overloaded ops should be desugared into a call).
+                this.cfg.push_assign(block, scope_id, expr_span, &lhs,
+                                     Rvalue::BinaryOp(op,
+                                                      Operand::Consume(lhs.clone()),
+                                                      rhs));
+
+                block.unit()
+            }
+            ExprKind::Continue { label } => {
+                this.break_or_continue(expr_span, label, block,
+                                       |loop_scope| loop_scope.continue_block)
+            }
+            ExprKind::Break { label } => {
+                this.break_or_continue(expr_span, label, block, |loop_scope| {
+                    loop_scope.might_break = true;
+                    loop_scope.break_block
+                })
+            }
+            ExprKind::Return { value } => {
+                block = match value {
+                    Some(value) => unpack!(this.into(&Lvalue::ReturnPointer, block, value)),
+                    None => {
+                        this.cfg.push_assign_unit(block, scope_id,
+                                                  expr_span, &Lvalue::ReturnPointer);
+                        block
+                    }
+                };
+                let extent = this.extent_of_return_scope();
+                let return_block = this.return_block();
+                this.exit_scope(expr_span, extent, block, return_block);
+                this.cfg.start_new_block().unit()
+            }
+            _ => {
+                let expr_span = expr.span;
+                let expr_ty = expr.ty;
+                let temp = this.temp(expr.ty.clone());
+                unpack!(block = this.into(&temp, block, expr));
+                unpack!(block = this.build_drop(block, expr_span, temp, expr_ty));
+                block.unit()
+            }
+        }
+    }
+
+    fn break_or_continue<F>(&mut self,
+                            span: Span,
+                            label: Option<CodeExtent>,
+                            block: BasicBlock,
+                            exit_selector: F)
+                            -> BlockAnd<()>
+        where F: FnOnce(&mut LoopScope) -> BasicBlock
+    {
+        let (exit_block, extent) = {
+            let loop_scope = self.find_loop_scope(span, label);
+            (exit_selector(loop_scope), loop_scope.extent)
+        };
+        self.exit_scope(span, extent, block, exit_block);
+        self.cfg.start_new_block().unit()
+    }
+
+}

src/librustc_trans/mir/analyze.rs

@@ -14,13 +14,13 @@
 use rustc_data_structures::bitvec::BitVector;
 use rustc::mir::repr as mir;
 use rustc::mir::visit::{Visitor, LvalueContext};
-use common::{self, Block};
+use common::{self, Block, BlockAndBuilder};
 use super::rvalue;
 
 pub fn lvalue_temps<'bcx,'tcx>(bcx: Block<'bcx,'tcx>,
-                               mir: &mir::Mir<'tcx>)
-                               -> BitVector {
-    let mut analyzer = TempAnalyzer::new(mir, bcx, mir.temp_decls.len());
+                               mir: &mir::Mir<'tcx>) -> BitVector {
+    let bcx = bcx.build();
+    let mut analyzer = TempAnalyzer::new(mir, &bcx, mir.temp_decls.len());
 
     analyzer.visit_mir(mir);
 
@@ -51,16 +51,16 @@ pub fn lvalue_temps<'bcx,'tcx>(bcx: Block<'bcx,'tcx>,
     analyzer.lvalue_temps
 }
 
-struct TempAnalyzer<'mir, 'bcx, 'tcx: 'mir + 'bcx> {
+struct TempAnalyzer<'mir, 'bcx: 'mir, 'tcx: 'bcx> {
     mir: &'mir mir::Mir<'tcx>,
-    bcx: Block<'bcx, 'tcx>,
+    bcx: &'mir BlockAndBuilder<'bcx, 'tcx>,
     lvalue_temps: BitVector,
     seen_assigned: BitVector
 }
 
 impl<'mir, 'bcx, 'tcx> TempAnalyzer<'mir, 'bcx, 'tcx> {
     fn new(mir: &'mir mir::Mir<'tcx>,
-           bcx: Block<'bcx, 'tcx>,
+           bcx: &'mir BlockAndBuilder<'bcx, 'tcx>,
            temp_count: usize) -> TempAnalyzer<'mir, 'bcx, 'tcx> {
         TempAnalyzer {
             mir: mir,

src/librustc_trans/mir/mod.rs

@@ -108,6 +108,16 @@ enum TempRef<'tcx> {
     Operand(Option<OperandRef<'tcx>>),
 }
 
+impl<'tcx> TempRef<'tcx> {
+    fn new_operand(val: OperandValue, ty: ty::Ty<'tcx>) -> TempRef<'tcx> {
+        let op = OperandRef {
+            val: val,
+            ty: ty
+        };
+        TempRef::Operand(Some(op))
+    }
+}
+
 ///////////////////////////////////////////////////////////////////////////
 
 pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) {
@@ -154,11 +164,8 @@ pub fn trans_mir<'blk, 'tcx: 'blk>(fcx: &'blk FunctionContext<'blk, 'tcx>) {
                                   // Zero-size temporaries aren't always initialized, which
                                   // doesn't matter because they don't contain data, but
                                   // we need something in the operand.
-                                  let op = OperandRef {
-                                      val: OperandValue::Immediate(common::C_nil(bcx.ccx())),
-                                      ty: mty
-                                  };
-                                  TempRef::Operand(Some(op))
+                                  let val = OperandValue::Immediate(common::C_nil(bcx.ccx()));
+                                  TempRef::new_operand(val, mty)
                               } else {
                                   // If this is an immediate temp, we do not create an
                                   // alloca in advance. Instead we wait until we see the

src/librustc_trans/mir/rvalue.rs

@@ -18,7 +18,7 @@ use rustc::mir::repr as mir;
 use asm;
 use base;
 use callee::Callee;
-use common::{self, C_uint, Block, BlockAndBuilder, Result};
+use common::{self, C_uint, BlockAndBuilder, Result};
 use datum::{Datum, Lvalue};
 use debuginfo::DebugLoc;
 use declare;
@@ -218,9 +218,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
             }
 
             _ => {
-                bcx.with_block(|bcx| {
-                    assert!(rvalue_creates_operand(&self.mir, bcx, rvalue));
-                });
+                assert!(rvalue_creates_operand(&self.mir, &bcx, rvalue));
                 let (bcx, temp) = self.trans_rvalue_operand(bcx, rvalue, debug_loc);
                 self.store_operand(&bcx, dest.llval, temp);
                 bcx
@@ -234,10 +232,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                                 debug_loc: DebugLoc)
                                 -> (BlockAndBuilder<'bcx, 'tcx>, OperandRef<'tcx>)
     {
-        bcx.with_block(|bcx| {
-            assert!(rvalue_creates_operand(&self.mir, bcx, rvalue),
-                    "cannot trans {:?} to operand", rvalue);
-        });
+        assert!(rvalue_creates_operand(&self.mir, &bcx, rvalue),
+                "cannot trans {:?} to operand", rvalue);
 
         match *rvalue {
             mir::Rvalue::Cast(ref kind, ref source, cast_ty) => {
@@ -608,7 +604,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
     }
 }
 
-pub fn rvalue_creates_operand<'bcx, 'tcx>(mir: &mir::Mir<'tcx>, bcx: Block<'bcx, 'tcx>,
+pub fn rvalue_creates_operand<'bcx, 'tcx>(mir: &mir::Mir<'tcx>,
+                                          bcx: &BlockAndBuilder<'bcx, 'tcx>,
                                           rvalue: &mir::Rvalue<'tcx>) -> bool {
     match *rvalue {
         mir::Rvalue::Ref(..) |

src/librustc_trans/mir/statement.rs

@@ -52,9 +52,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                                 } else {
                                     // If the type is zero-sized, it's already been set here,
                                     // but we still need to make sure we translate the operand
-                                    let (bcx, _) = self.trans_rvalue_operand(bcx, rvalue,
-                                                                                   debug_loc);
-                                    bcx
+                                    self.trans_rvalue_operand(bcx, rvalue, debug_loc).0
                                 }
                             }
                         }