Apply @nikomatsakis' nits and comments patch.

8 files changed, 149 insertions, 83 deletions

diff --git a/src/librustc/middle/trans/callee.rs b/src/librustc/middle/trans/callee.rs
index 5e2a8792abf..ff63b74444c 100644
--- a/src/librustc/middle/trans/callee.rs
+++ b/src/librustc/middle/trans/callee.rs
@@ -173,10 +173,13 @@ pub fn trans_fn_ref(bcx: &Block, def_id: ast::DefId, node: ExprOrMethodCall) ->
     let type_params = node_id_type_params(bcx, node);
     let vtables = match node {
         ExprId(id) => node_vtables(bcx, id),
-        MethodCall(method_call) if method_call.autoderef == 0 => {
-            node_vtables(bcx, method_call.expr_id)
+        MethodCall(ref method_call) => {
+            if method_call.autoderef == 0 {
+                node_vtables(bcx, method_call.expr_id)
+            } else {
+                None
+            }
         }
-        _ => None
     };
     debug!("trans_fn_ref(def_id={}, node={:?}, type_params={}, vtables={})",
            def_id.repr(bcx.tcx()), node, type_params.repr(bcx.tcx()),
@@ -381,15 +384,15 @@ pub fn trans_fn_ref_with_vtables(
         // Should be either intra-crate or inlined.
         assert_eq!(def_id.krate, ast::LOCAL_CRATE);
 
-        let ref_id = match node {
-            ExprId(id) if id != 0 => Some(id),
-            _ => None
+        let opt_ref_id = match node {
+            ExprId(id) => if id != 0 { Some(id) } else { None },
+            MethodCall(_) => None,
         };
 
         let (val, must_cast) =
             monomorphize::monomorphic_fn(ccx, def_id, &substs,
                                          vtables, self_vtables,
-                                         ref_id);
+                                         opt_ref_id);
         let mut val = val;
         if must_cast && node != ExprId(0) {
             // Monotype of the REFERENCE to the function (type params
@@ -758,9 +761,19 @@ pub fn trans_call_inner<'a>(
 }
 
 pub enum CallArgs<'a> {
+    // Supply value of arguments as a list of expressions that must be
+    // translated. This is used in the common case of `foo(bar, qux)`.
     ArgExprs(&'a [@ast::Expr]),
+
+    // Supply value of arguments as a list of LLVM value refs; frequently
+    // used with lang items and so forth, when the argument is an internal
+    // value.
+    ArgVals(&'a [ValueRef]),
+
+    // For overloaded operators: `(lhs, Option(rhs, rhs_id))`. `lhs`
+    // is the left-hand-side and `rhs/rhs_id` is the datum/expr-id of
+    // the right-hand-side (if any).
     ArgOverloadedOp(Datum<Expr>, Option<(Datum<Expr>, ast::NodeId)>),
-    ArgVals(&'a [ValueRef])
 }
 
 fn trans_args<'a>(cx: &'a Block<'a>,
diff --git a/src/librustc/middle/trans/common.rs b/src/librustc/middle/trans/common.rs
index 759e5e872d4..e3a996e33d3 100644
--- a/src/librustc/middle/trans/common.rs
+++ b/src/librustc/middle/trans/common.rs
@@ -809,9 +809,13 @@ pub fn expr_ty_adjusted(bcx: &Block, ex: &ast::Expr) -> ty::t {
     monomorphize_type(bcx, t)
 }
 
+// Key used to lookup values supplied for type parameters in an expr.
 #[deriving(Eq)]
 pub enum ExprOrMethodCall {
+    // Type parameters for a path like `None::<int>`
     ExprId(ast::NodeId),
+
+    // Type parameters for a method call like `a.foo::<int>()`
     MethodCall(typeck::MethodCall)
 }
 
diff --git a/src/librustc/middle/trans/expr.rs b/src/librustc/middle/trans/expr.rs
index 056ac62183f..d143d674305 100644
--- a/src/librustc/middle/trans/expr.rs
+++ b/src/librustc/middle/trans/expr.rs
@@ -1160,9 +1160,13 @@ fn trans_unary<'a>(bcx: &'a Block<'a>,
     let _icx = push_ctxt("trans_unary_datum");
 
     let method_call = MethodCall::expr(expr.id);
-    let overloaded = bcx.ccx().maps.method_map.borrow().get().contains_key(&method_call);
-    // if overloaded, would be RvalueDpsExpr
-    assert!(!overloaded || op == ast::UnDeref);
+
+    // The only overloaded operator that is translated to a datum
+    // is an overloaded deref, since it is always yields a `&T`.
+    // Otherwise, we should be in the RvalueDpsExpr path.
+    assert!(
+        op == ast::UnDeref ||
+        !bcx.ccx().maps.method_map.borrow().get().contains_key(&method_call));
 
     let un_ty = expr_ty(bcx, expr);
 
@@ -1779,6 +1783,7 @@ fn deref_once<'a>(bcx: &'a Block<'a>,
 
     let mut bcx = bcx;
 
+    // Check for overloaded deref.
     let method_call = MethodCall {
         expr_id: expr.id,
         autoderef: derefs as u32
@@ -1787,6 +1792,11 @@ fn deref_once<'a>(bcx: &'a Block<'a>,
                        .find(&method_call).map(|method| method.ty);
     let datum = match method_ty {
         Some(method_ty) => {
+            // Overloaded. Evaluate `trans_overloaded_op`, which will
+            // invoke the user's deref() method, which basically
+            // converts from the `Shaht<T>` pointer that we have into
+            // a `&T` pointer.  We can then proceed down the normal
+            // path (below) to dereference that `&T`.
             let datum = if derefs == 0 {
                 datum
             } else {
@@ -1798,7 +1808,10 @@ fn deref_once<'a>(bcx: &'a Block<'a>,
             let ref_ty = ty::ty_fn_ret(monomorphize_type(bcx, method_ty));
             Datum(val, ref_ty, RvalueExpr(Rvalue(ByValue)))
         }
-        None => datum
+        None => {
+            // Not overloaded. We already have a pointer we know how to deref.
+            datum
+        }
     };
 
     let r = match ty::get(datum.ty).sty {
diff --git a/src/librustc/middle/trans/meth.rs b/src/librustc/middle/trans/meth.rs
index b3d69043d33..89f33776431 100644
--- a/src/librustc/middle/trans/meth.rs
+++ b/src/librustc/middle/trans/meth.rs
@@ -335,10 +335,13 @@ fn combine_impl_and_methods_tps(bcx: &Block,
     // exist, in which case we need to make them.
     let vtables = match node {
         ExprId(id) => node_vtables(bcx, id),
-        MethodCall(method_call) if method_call.autoderef == 0 => {
-            node_vtables(bcx, method_call.expr_id)
+        MethodCall(method_call) => {
+            if method_call.autoderef == 0 {
+                node_vtables(bcx, method_call.expr_id)
+            } else {
+                None
+            }
         }
-        _ => None
     };
     let r_m_origins = match vtables {
         Some(vt) => vt,
diff --git a/src/librustc/middle/ty.rs b/src/librustc/middle/ty.rs
index 9d0fb866916..c0ad18d9520 100644
--- a/src/librustc/middle/ty.rs
+++ b/src/librustc/middle/ty.rs
@@ -229,7 +229,7 @@ pub struct AutoDerefRef {
     autoref: Option<AutoRef>
 }
 
-#[deriving(Decodable, Encodable)]
+#[deriving(Decodable, Encodable, Eq, Show)]
 pub enum AutoRef {
     /// Convert from T to &T
     AutoPtr(Region, ast::Mutability),
@@ -3271,11 +3271,15 @@ pub fn expr_kind(tcx: ctxt,
                  expr: &ast::Expr) -> ExprKind {
     if method_map.borrow().get().contains_key(&MethodCall::expr(expr.id)) {
         // Overloaded operations are generally calls, and hence they are
-        // generated via DPS.  However, assign_op (e.g., `x += y`) is an
-        // exception, as its result is always unit.
+        // generated via DPS, but there are two exceptions:
         return match expr.node {
+            // `a += b` has a unit result.
             ast::ExprAssignOp(..) => RvalueStmtExpr,
+
+            // the deref method invoked for `*a` always yields an `&T`
             ast::ExprUnary(ast::UnDeref, _) => LvalueExpr,
+
+            // in the general case, result could be any type, use DPS
             _ => RvalueDpsExpr
         };
     }
diff --git a/src/librustc/middle/typeck/check/method.rs b/src/librustc/middle/typeck/check/method.rs
index 89658b32be8..0d37904445f 100644
--- a/src/librustc/middle/typeck/check/method.rs
+++ b/src/librustc/middle/typeck/check/method.rs
@@ -201,7 +201,13 @@ pub fn lookup_in_trait<'a>(
 struct LookupContext<'a> {
     fcx: @FnCtxt,
     span: Span,
+
+    // The receiver to the method call. Only `None` in the case of
+    // an overloaded autoderef, where the receiver may be an intermediate
+    // state like "the expression `x` when it has been autoderef'd
+    // twice already".
     self_expr: Option<&'a ast::Expr>,
+
     m_name: ast::Name,
     supplied_tps: &'a [ty::t],
     impl_dups: @RefCell<HashSet<DefId>>,
@@ -243,51 +249,69 @@ impl<'a> LookupContext<'a> {
         let span = self.self_expr.map_or(self.span, |e| e.span);
         let self_expr_id = self.self_expr.map(|e| e.id);
         let (self_ty, autoderefs, result) =
-            check::autoderef(self.fcx, span, self_ty, self_expr_id,
-                             PreferMutLvalue, |self_ty, autoderefs| {
-
-            debug!("loop: self_ty={} autoderefs={}",
-                   self.ty_to_str(self_ty), autoderefs);
+            check::autoderef(
+                self.fcx, span, self_ty, self_expr_id, PreferMutLvalue,
+                |self_ty, autoderefs| self.search_step(self_ty, autoderefs));
 
-            match self.deref_args {
-                check::DontDerefArgs => {
-                    match self.search_for_autoderefd_method(self_ty, autoderefs) {
-                        Some(result) => return Some(Some(result)),
-                        None => {}
-                    }
+        match result {
+            Some(Some(result)) => Some(result),
+            _ => {
+                if self.is_overloaded_deref() {
+                    // If we are searching for an overloaded deref, no
+                    // need to try coercing a `~[T]` to an `&[T]` and
+                    // searching for an overloaded deref on *that*.
+                    None
+                } else {
+                    self.search_for_autosliced_method(self_ty, autoderefs)
+                }
+            }
+        }
+    }
 
-                    match self.search_for_autoptrd_method(self_ty, autoderefs) {
-                        Some(result) => return Some(Some(result)),
-                        None => {}
-                    }
+    fn search_step(&self,
+                   self_ty: ty::t,
+                   autoderefs: uint)
+                   -> Option<Option<MethodCallee>> {
+        debug!("search_step: self_ty={} autoderefs={}",
+               self.ty_to_str(self_ty), autoderefs);
+
+        match self.deref_args {
+            check::DontDerefArgs => {
+                match self.search_for_autoderefd_method(self_ty, autoderefs) {
+                    Some(result) => return Some(Some(result)),
+                    None => {}
                 }
-                check::DoDerefArgs => {
-                    match self.search_for_autoptrd_method(self_ty, autoderefs) {
-                        Some(result) => return Some(Some(result)),
-                        None => {}
-                    }
 
-                    match self.search_for_autoderefd_method(self_ty, autoderefs) {
-                        Some(result) => return Some(Some(result)),
-                        None => {}
-                    }
+                match self.search_for_autoptrd_method(self_ty, autoderefs) {
+                    Some(result) => return Some(Some(result)),
+                    None => {}
                 }
             }
+            check::DoDerefArgs => {
+                match self.search_for_autoptrd_method(self_ty, autoderefs) {
+                    Some(result) => return Some(Some(result)),
+                    None => {}
+                }
 
-            // Don't autoderef if we aren't supposed to.
-            if self.autoderef_receiver == DontAutoderefReceiver {
-                Some(None)
-            } else {
-                None
+                match self.search_for_autoderefd_method(self_ty, autoderefs) {
+                    Some(result) => return Some(Some(result)),
+                    None => {}
+                }
             }
-        });
+        }
 
-        match result {
-            Some(Some(result)) => Some(result),
-            _ => self.search_for_autosliced_method(self_ty, autoderefs)
+        // Don't autoderef if we aren't supposed to.
+        if self.autoderef_receiver == DontAutoderefReceiver {
+            Some(None)
+        } else {
+            None
         }
     }
 
+    fn is_overloaded_deref(&self) -> bool {
+        self.self_expr.is_none()
+    }
+
     // ______________________________________________________________________
     // Candidate collection (see comment at start of file)
 
@@ -625,17 +649,13 @@ impl<'a> LookupContext<'a> {
         let (self_ty, auto_deref_ref) =
             self.consider_reborrow(self_ty, autoderefs);
 
-        // HACK(eddyb) only overloaded auto-deref calls should be missing
-        // adjustments, because we imply an AutoPtr adjustment for them.
-        let adjustment = match auto_deref_ref {
-            ty::AutoDerefRef {
-                autoderefs: 0,
-                autoref: Some(ty::AutoPtr(..))
-            } => None,
-            _ => match self.self_expr {
-                Some(expr) => Some((expr.id, @ty::AutoDerefRef(auto_deref_ref))),
-                None => return None
-            }
+        // Hacky. For overloaded derefs, there may be an adjustment
+        // added to the expression from the outside context, so we do not store
+        // an explicit adjustment, but rather we hardwire the single deref
+        // that occurs in trans and mem_categorization.
+        let adjustment = match self.self_expr {
+            Some(expr) => Some((expr.id, @ty::AutoDerefRef(auto_deref_ref))),
+            None => return None
         };
 
         match self.search_for_method(self_ty) {
@@ -733,9 +753,9 @@ impl<'a> LookupContext<'a> {
                                     autoderefs: uint)
                                     -> Option<MethodCallee> {
         /*!
-         *
          * Searches for a candidate by converting things like
-         * `~[]` to `&[]`. */
+         * `~[]` to `&[]`.
+         */
 
         let tcx = self.tcx();
         let sty = ty::get(self_ty).sty.clone();
@@ -843,15 +863,20 @@ impl<'a> LookupContext<'a> {
             mutbls: &[ast::Mutability],
             mk_autoref_ty: |ast::Mutability, ty::Region| -> ty::t)
             -> Option<MethodCallee> {
-        // HACK(eddyb) only overloaded auto-deref calls should be missing
-        // adjustments, because we imply an AutoPtr adjustment for them.
+        // Hacky. For overloaded derefs, there may be an adjustment
+        // added to the expression from the outside context, so we do not store
+        // an explicit adjustment, but rather we hardwire the single deref
+        // that occurs in trans and mem_categorization.
         let self_expr_id = match self.self_expr {
             Some(expr) => Some(expr.id),
-            None => match kind(ty::ReEmpty, ast::MutImmutable) {
-                ty::AutoPtr(..) if autoderefs == 0 => None,
-                _ => return None
+            None => {
+                assert_eq!(autoderefs, 0);
+                assert_eq!(kind(ty::ReEmpty, ast::MutImmutable),
+                           ty::AutoPtr(ty::ReEmpty, ast::MutImmutable));
+                None
             }
         };
+
         // This is hokey. We should have mutability inference as a
         // variable.  But for now, try &const, then &, then &mut:
         let region =
@@ -1119,7 +1144,8 @@ impl<'a> LookupContext<'a> {
         &self,
         trait_def_id: ast::DefId,
         rcvr_substs: &ty::substs,
-        method_ty: &ty::Method) -> ty::t {
+        method_ty: &ty::Method)
+        -> ty::t {
         /*!
          * This is a bit tricky. We have a match against a trait method
          * being invoked on an object, and we want to generate the
diff --git a/src/librustc/middle/typeck/check/mod.rs b/src/librustc/middle/typeck/check/mod.rs
index e3db7f16064..51efcb7d1c3 100644
--- a/src/librustc/middle/typeck/check/mod.rs
+++ b/src/librustc/middle/typeck/check/mod.rs
@@ -1247,11 +1247,14 @@ pub fn autoderef<T>(fcx: @FnCtxt, sp: Span, base_ty: ty::t,
                     should_stop: |ty::t, uint| -> Option<T>)
                     -> (ty::t, uint, Option<T>) {
     /*!
+     * Executes an autoderef loop for the type `t`. At each step, invokes
+     * `should_stop` to decide whether to terminate the loop. Returns
+     * the final type and number of derefs that it performed.
      *
-     * Autoderefs the type `t` as many times as possible, returning a new type
-     * and an autoderef count. If the count is not zero, the receiver is
-     * responsible for inserting an AutoAdjustment record into `tcx.adjustments`
-     * so that trans/borrowck/etc know about this autoderef. */
+     * Note: this method does not modify the adjustments table. The caller is
+     * responsible for inserting an AutoAdjustment record into the `fcx`
+     * using one of the suitable methods.
+     */
 
     let mut t = base_ty;
     for autoderefs in range(0, fcx.tcx().sess.recursion_limit.get()) {
@@ -2282,15 +2285,15 @@ fn check_expr_with_unifier(fcx: @FnCtxt,
         // FIXME(eddyb) #12808 Integrate privacy into this auto-deref loop.
         let (_, autoderefs, field_ty) =
             autoderef(fcx, expr.span, expr_t, Some(base.id), lvalue_pref, |base_t, _| {
-            match ty::get(base_t).sty {
-                ty::ty_struct(base_id, ref substs) => {
-                    debug!("struct named {}", ppaux::ty_to_str(tcx, base_t));
-                    let fields = ty::lookup_struct_fields(tcx, base_id);
-                    lookup_field_ty(tcx, base_id, fields.as_slice(), field, &(*substs))
+                match ty::get(base_t).sty {
+                    ty::ty_struct(base_id, ref substs) => {
+                        debug!("struct named {}", ppaux::ty_to_str(tcx, base_t));
+                        let fields = ty::lookup_struct_fields(tcx, base_id);
+                        lookup_field_ty(tcx, base_id, fields.as_slice(), field, &(*substs))
+                    }
+                    _ => None
                 }
-                _ => None
-            }
-        });
+            });
         match field_ty {
             Some(field_ty) => {
                 fcx.write_ty(expr.id, field_ty);
diff --git a/src/libsyntax/ast.rs b/src/libsyntax/ast.rs
index b6b18f6671d..7fef6da5607 100644
--- a/src/libsyntax/ast.rs
+++ b/src/libsyntax/ast.rs
@@ -354,7 +354,7 @@ pub enum Pat_ {
     PatVec(Vec<@Pat> , Option<@Pat>, Vec<@Pat> )
 }
 
-#[deriving(Clone, Eq, Encodable, Decodable, Hash)]
+#[deriving(Clone, Eq, Encodable, Decodable, Hash, Show)]
 pub enum Mutability {
     MutMutable,
     MutImmutable,