address review comments

4 files changed, 98 insertions, 74 deletions

diff --git a/src/librustc_mir/tcx/mod.rs b/src/librustc_mir/tcx/mod.rs
index 69240e9f995..15a49fc9d85 100644
--- a/src/librustc_mir/tcx/mod.rs
+++ b/src/librustc_mir/tcx/mod.rs
@@ -103,6 +103,31 @@ impl<'tcx> Mir<'tcx> {
         }
     }
 
+    pub fn binop_ty(&self,
+                    tcx: &ty::ctxt<'tcx>,
+                    op: BinOp,
+                    lhs_ty: Ty<'tcx>,
+                    rhs_ty: Ty<'tcx>)
+                    -> Ty<'tcx>
+    {
+        // FIXME: handle SIMD correctly
+        match op {
+            BinOp::Add | BinOp::Sub | BinOp::Mul | BinOp::Div | BinOp::Rem |
+            BinOp::BitXor | BinOp::BitAnd | BinOp::BitOr => {
+                // these should be integers or floats of the same size.
+                assert_eq!(lhs_ty, rhs_ty);
+                lhs_ty
+            }
+            BinOp::Shl | BinOp::Shr => {
+                lhs_ty // lhs_ty can be != rhs_ty
+            }
+            BinOp::Eq | BinOp::Lt | BinOp::Le |
+            BinOp::Ne | BinOp::Ge | BinOp::Gt => {
+                tcx.types.bool
+            }
+        }
+    }
+
     pub fn lvalue_ty(&self,
                      tcx: &ty::ctxt<'tcx>,
                      lvalue: &Lvalue<'tcx>)
@@ -138,3 +163,26 @@ impl BorrowKind {
         }
     }
 }
+
+impl BinOp {
+    pub fn to_hir_binop(self) -> hir::BinOp_ {
+        match self {
+            BinOp::Add => hir::BinOp_::BiAdd,
+            BinOp::Sub => hir::BinOp_::BiSub,
+            BinOp::Mul => hir::BinOp_::BiMul,
+            BinOp::Div => hir::BinOp_::BiDiv,
+            BinOp::Rem => hir::BinOp_::BiRem,
+            BinOp::BitXor => hir::BinOp_::BiBitXor,
+            BinOp::BitAnd => hir::BinOp_::BiBitAnd,
+            BinOp::BitOr => hir::BinOp_::BiBitOr,
+            BinOp::Shl => hir::BinOp_::BiShl,
+            BinOp::Shr => hir::BinOp_::BiShr,
+            BinOp::Eq => hir::BinOp_::BiEq,
+            BinOp::Ne => hir::BinOp_::BiNe,
+            BinOp::Lt => hir::BinOp_::BiLt,
+            BinOp::Gt => hir::BinOp_::BiGt,
+            BinOp::Le => hir::BinOp_::BiLe,
+            BinOp::Ge => hir::BinOp_::BiGe
+        }
+    }
+}
diff --git a/src/librustc_trans/trans/mir/constant.rs b/src/librustc_trans/trans/mir/constant.rs
index 923baf0dcfe..8c0d8b10bfe 100644
--- a/src/librustc_trans/trans/mir/constant.rs
+++ b/src/librustc_trans/trans/mir/constant.rs
@@ -16,7 +16,7 @@ use trans::common::{self, Block};
 use trans::common::{C_bool, C_bytes, C_floating_f64, C_integral, C_str_slice};
 use trans::type_of;
 
-use super::operand::{OperandRef, OperandValue};
+use super::operand::OperandRef;
 use super::MirContext;
 
 impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
@@ -26,19 +26,21 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                           ty: Ty<'tcx>)
                           -> OperandRef<'tcx>
     {
+        use super::operand::OperandValue::{Ref, Immediate};
+
         let ccx = bcx.ccx();
         let llty = type_of::type_of(ccx, ty);
         let val = match *cv {
-            ConstVal::Float(v) => OperandValue::Imm(C_floating_f64(v, llty)),
-            ConstVal::Bool(v) => OperandValue::Imm(C_bool(ccx, v)),
-            ConstVal::Int(v) => OperandValue::Imm(C_integral(llty, v as u64, true)),
-            ConstVal::Uint(v) => OperandValue::Imm(C_integral(llty, v, false)),
-            ConstVal::Str(ref v) => OperandValue::Imm(C_str_slice(ccx, v.clone())),
+            ConstVal::Float(v) => Immediate(C_floating_f64(v, llty)),
+            ConstVal::Bool(v) => Immediate(C_bool(ccx, v)),
+            ConstVal::Int(v) => Immediate(C_integral(llty, v as u64, true)),
+            ConstVal::Uint(v) => Immediate(C_integral(llty, v, false)),
+            ConstVal::Str(ref v) => Immediate(C_str_slice(ccx, v.clone())),
             ConstVal::ByteStr(ref v) => {
-                OperandValue::Imm(consts::addr_of(ccx,
-                                                  C_bytes(ccx, v),
-                                                  1,
-                                                  "byte_str"))
+                Immediate(consts::addr_of(ccx,
+                                          C_bytes(ccx, v),
+                                          1,
+                                          "byte_str"))
             }
 
             ConstVal::Struct(id) | ConstVal::Tuple(id) => {
@@ -52,9 +54,9 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                     Err(_) => panic!("constant eval failure"),
                 };
                 if common::type_is_immediate(bcx.ccx(), ty) {
-                    OperandValue::Imm(llval)
+                    Immediate(llval)
                 } else {
-                    OperandValue::Ref(llval)
+                    Ref(llval)
                 }
             }
             ConstVal::Function(_) => {
diff --git a/src/librustc_trans/trans/mir/operand.rs b/src/librustc_trans/trans/mir/operand.rs
index 7daf76a8d31..63abdfe2dd9 100644
--- a/src/librustc_trans/trans/mir/operand.rs
+++ b/src/librustc_trans/trans/mir/operand.rs
@@ -17,8 +17,8 @@ use trans::datum;
 
 use super::{MirContext, TempRef};
 
-/// The Rust representation of an operand's value. This is uniquely
-/// determined by the operand type, but is kept as an enum as a
+/// The representation of a Rust value. The enum variant is in fact
+/// uniquely determined by the value's type, but is kept as a
 /// safety check.
 #[derive(Copy, Clone)]
 pub enum OperandValue {
@@ -26,16 +26,22 @@ pub enum OperandValue {
     /// to be valid for the operand's lifetime.
     Ref(ValueRef),
     /// A single LLVM value.
-    Imm(ValueRef),
+    Immediate(ValueRef),
     /// A fat pointer. The first ValueRef is the data and the second
     /// is the extra.
     FatPtr(ValueRef, ValueRef)
 }
 
+/// An `OperandRef` is an "SSA" reference to a Rust value, along with
+/// its type.
+///
+/// NOTE: unless you know a value's type exactly, you should not
+/// generate LLVM opcodes acting on it and instead act via methods,
+/// to avoid nasty edge cases. In particular, using `build::Store`
+/// directly is sure to cause problems - use `store_operand` instead.
 #[derive(Copy, Clone)]
 pub struct OperandRef<'tcx> {
-    // This will be "indirect" if `appropriate_rvalue_mode` returns
-    // ByRef, and otherwise ByValue.
+    // The value.
     pub val: OperandValue,
 
     // The type of value being returned.
@@ -43,9 +49,11 @@ pub struct OperandRef<'tcx> {
 }
 
 impl<'tcx> OperandRef<'tcx> {
+    /// Asserts that this operand refers to a scalar and returns
+    /// a reference to its value.
     pub fn immediate(self) -> ValueRef {
         match self.val {
-            OperandValue::Imm(s) => s,
+            OperandValue::Immediate(s) => s,
             _ => unreachable!()
         }
     }
@@ -56,8 +64,8 @@ impl<'tcx> OperandRef<'tcx> {
                 format!("OperandRef(Ref({}) @ {:?})",
                         bcx.val_to_string(r), self.ty)
             }
-            OperandValue::Imm(i) => {
-                format!("OperandRef(Imm({}) @ {:?})",
+            OperandValue::Immediate(i) => {
+                format!("OperandRef(Immediate({}) @ {:?})",
                         bcx.val_to_string(i), self.ty)
             }
             OperandValue::FatPtr(a, d) => {
@@ -106,7 +114,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                        ty);
                 let val = match datum::appropriate_rvalue_mode(bcx.ccx(), ty) {
                     datum::ByValue => {
-                        OperandValue::Imm(base::load_ty(bcx, tr_lvalue.llval, ty))
+                        OperandValue::Immediate(base::load_ty(bcx, tr_lvalue.llval, ty))
                     }
                     datum::ByRef if common::type_is_fat_ptr(bcx.tcx(), ty) => {
                         let (lldata, llextra) = base::load_fat_ptr(bcx, tr_lvalue.llval, ty);
@@ -150,7 +158,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
         debug!("store_operand: operand={}", operand.repr(bcx));
         match operand.val {
             OperandValue::Ref(r) => base::memcpy_ty(bcx, lldest, r, operand.ty),
-            OperandValue::Imm(s) => base::store_ty(bcx, s, lldest, operand.ty),
+            OperandValue::Immediate(s) => base::store_ty(bcx, s, lldest, operand.ty),
             OperandValue::FatPtr(data, extra) => {
                 base::store_fat_ptr(bcx, data, extra, lldest, operand.ty);
             }
diff --git a/src/librustc_trans/trans/mir/rvalue.rs b/src/librustc_trans/trans/mir/rvalue.rs
index 8f5496929c2..cce71b25702 100644
--- a/src/librustc_trans/trans/mir/rvalue.rs
+++ b/src/librustc_trans/trans/mir/rvalue.rs
@@ -10,7 +10,6 @@
 
 use llvm::ValueRef;
 use rustc::middle::ty::{self, Ty};
-use rustc_front::hir;
 use rustc_mir::repr as mir;
 
 use trans::asm;
@@ -47,6 +46,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
 
             mir::Rvalue::Cast(mir::CastKind::Unsize, ref operand, cast_ty) => {
                 if common::type_is_fat_ptr(bcx.tcx(), cast_ty) {
+                    // into-coerce of a thin pointer to a fat pointer - just
+                    // use the operand path.
                     let (bcx, temp) = self.trans_rvalue_operand(bcx, rvalue);
                     self.store_operand(bcx, lldest, temp);
                     return bcx;
@@ -59,8 +60,13 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                 let operand = self.trans_operand(bcx, operand);
                 match operand.val {
                     OperandValue::FatPtr(..) => unreachable!(),
-                    OperandValue::Imm(llval) => {
-                        // ugly alloca.
+                    OperandValue::Immediate(llval) => {
+                        // unsize from an immediate structure. We don't
+                        // really need a temporary alloca here, but
+                        // avoiding it would require us to have
+                        // `coerce_unsized_into` use extractvalue to
+                        // index into the struct, and this case isn't
+                        // important enough for it.
                         debug!("trans_rvalue: creating ugly alloca");
                         let lltemp = base::alloc_ty(bcx, operand.ty, "__unsize_temp");
                         base::store_ty(bcx, llval, lltemp, operand.ty);
@@ -165,7 +171,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                                 // and is a no-op at the LLVM level
                                 operand.val
                             }
-                            OperandValue::Imm(lldata) => {
+                            OperandValue::Immediate(lldata) => {
                                 // "standard" unsize
                                 let (lldata, llextra) =
                                     base::unsize_thin_ptr(bcx, lldata,
@@ -200,7 +206,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                 // destination effectively creates a reference.
                 if common::type_is_sized(bcx.tcx(), ty) {
                     (bcx, OperandRef {
-                        val: OperandValue::Imm(tr_lvalue.llval),
+                        val: OperandValue::Immediate(tr_lvalue.llval),
                         ty: ref_ty,
                     })
                 } else {
@@ -215,7 +221,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
             mir::Rvalue::Len(ref lvalue) => {
                 let tr_lvalue = self.trans_lvalue(bcx, lvalue);
                 (bcx, OperandRef {
-                    val: OperandValue::Imm(self.lvalue_len(bcx, tr_lvalue)),
+                    val: OperandValue::Immediate(self.lvalue_len(bcx, tr_lvalue)),
                     ty: bcx.tcx().types.usize,
                 })
             }
@@ -230,7 +236,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                             base::compare_fat_ptrs(bcx,
                                                    lhs_addr, lhs_extra,
                                                    rhs_addr, rhs_extra,
-                                                   lhs.ty, cmp_to_hir_cmp(op),
+                                                   lhs.ty, op.to_hir_binop(),
                                                    DebugLoc::None)
                         }
                         _ => unreachable!()
@@ -242,8 +248,8 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                                             lhs.ty, DebugLoc::None)
                 };
                 (bcx, OperandRef {
-                    val: OperandValue::Imm(llresult),
-                    ty: type_of_binop(bcx.tcx(), op, lhs.ty, rhs.ty),
+                    val: OperandValue::Immediate(llresult),
+                    ty: self.mir.binop_ty(bcx.tcx(), op, lhs.ty, rhs.ty),
                 })
             }
 
@@ -261,7 +267,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                     }
                 };
                 (bcx, OperandRef {
-                    val: OperandValue::Imm(llval),
+                    val: OperandValue::Immediate(llval),
                     ty: operand.ty,
                 })
             }
@@ -281,7 +287,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
                                                                       llalign,
                                                                       DebugLoc::None);
                 (bcx, OperandRef {
-                    val: OperandValue::Imm(llval),
+                    val: OperandValue::Immediate(llval),
                     ty: box_ty,
                 })
             }
@@ -388,7 +394,7 @@ impl<'bcx, 'tcx> MirContext<'bcx, 'tcx> {
             mir::BinOp::Eq | mir::BinOp::Lt | mir::BinOp::Gt |
             mir::BinOp::Ne | mir::BinOp::Le | mir::BinOp::Ge => {
                 base::compare_scalar_types(bcx, lhs, rhs, input_ty,
-                                           cmp_to_hir_cmp(op), debug_loc)
+                                           op.to_hir_binop(), debug_loc)
             }
         }
     }
@@ -413,43 +419,3 @@ pub fn rvalue_creates_operand<'tcx>(rvalue: &mir::Rvalue<'tcx>) -> bool {
 
     // (*) this is only true if the type is suitable
 }
-
-fn cmp_to_hir_cmp(op: mir::BinOp) -> hir::BinOp_ {
-    match op {
-        mir::BinOp::Eq => hir::BiEq,
-        mir::BinOp::Ne => hir::BiNe,
-        mir::BinOp::Lt => hir::BiLt,
-        mir::BinOp::Le => hir::BiLe,
-        mir::BinOp::Gt => hir::BiGt,
-        mir::BinOp::Ge => hir::BiGe,
-        _ => unreachable!()
-    }
-}
-
-/// FIXME(nikomatsakis): I don't think this function should go here
-fn type_of_binop<'tcx>(
-    tcx: &ty::ctxt<'tcx>,
-    op: mir::BinOp,
-    lhs_ty: Ty<'tcx>,
-    rhs_ty: Ty<'tcx>)
-    -> Ty<'tcx>
-{
-    match op {
-        mir::BinOp::Add | mir::BinOp::Sub |
-        mir::BinOp::Mul | mir::BinOp::Div | mir::BinOp::Rem |
-        mir::BinOp::BitXor | mir::BinOp::BitAnd | mir::BinOp::BitOr => {
-            // these should be integers or floats of the same size. We
-            // probably want to dump all ops in some intrinsics framework
-            // someday.
-            assert_eq!(lhs_ty, rhs_ty);
-            lhs_ty
-        }
-        mir::BinOp::Shl | mir::BinOp::Shr => {
-            lhs_ty // lhs_ty can be != rhs_ty
-        }
-        mir::BinOp::Eq | mir::BinOp::Lt | mir::BinOp::Le |
-        mir::BinOp::Ne | mir::BinOp::Ge | mir::BinOp::Gt => {
-            tcx.types.bool
-        }
-    }
-}