Sema: avoid reuse of Exprs when synthesizing operator=

[labath]

Summary

Previously, Sema was reusing parts of the AST when synthesizing an assignment
operator, turning it into a AS-dag. This caused problems for the static
analyzer, which assumed an expression appears in the tree only once.

Here I make sure to always create a fresh Expr, when inserting something into
the AST, fixing PR16745 in the process.

[klimek]

lib/Sema/SemaDeclCXX.cpp

8493 ↗

(On Diff #3527)

I personally find a class structure easier to understand that goes from public: over protected: to private:, and has methods ordered from most important to know for a user of the class to least important, as that aids the common top-down reading flow.

This file is inconsistent regarding this style question, so I punt to doug for judgement ;)

8505 ↗

(On Diff #3527)

s/cannot/must not/
Also, I'd probably pull the asserts out of this method. It inserts some duplication, but it also simplifies this class hierarchy.

8514–8518 ↗

(On Diff #3527)

I think at some point the reduction in duplication gets outweighed by the increase in structural overhead. This might be a different call if not all implementations were here in the .cc file, but in this case I'd just have the Build method on the interface and have an implementation on each class. If you want to pull out common code, I'd put helpers into the base class.

8867 ↗

(On Diff #3527)

I'd s/Idx/Index/

8871 ↗

(On Diff #3527)

That cast seems wrong. Please try to reformulate without the cast. Use pointers if necessary.

[labath]

• Minor refactoring to adress Manuel's comments

lib/Sema/SemaDeclCXX.cpp

8493 ↗

(On Diff #3527)

Ok, I'm leaving the members in the present order for now.

8505 ↗

(On Diff #3527)

I've put the assertion into a helper method.

8514–8518 ↗

(On Diff #3527)

I've removed this intermediate class.

8867 ↗

(On Diff #3527)

Usage of "Index" seems to be more common in this file, replaced.

8871 ↗

(On Diff #3527)

Reformulated.

[klimek]

Apart from the backwards-layout of classes (which makes it hard to read for me) makes sense. Needs review from Richard or Doug for domain knowledge...

[rsmith]

Seems fine to me.

lib/Sema/SemaDeclCXX.cpp

8479 ↗

(On Diff #3798)

No space between operator and =

[labath]

Closed by commit rL189659 (authored by @labath).

cfe/trunk/lib/Sema/SemaDeclCXX.cpp

void Sema::AdjustDestructorExceptionSpec(CXXRecordDecl *ClassDecl,
 
   // FIXME: If the destructor has a body that could throw, and the newly created
   // spec doesn't allow exceptions, we should emit a warning, because this
   // change in behavior can break conforming C++03 programs at runtime.
   // However, we don't have a body or an exception specification yet, so it
   // needs to be done somewhere else.
 }
 
+namespace {
+/// \brief An abstract base class for all helper classes used in building the
+//  copy/move operators. These classes serve as factory functions and help us
+//  avoid using the same Expr* in the AST twice.
+class ExprBuilder {
+  ExprBuilder(const ExprBuilder&) LLVM_DELETED_FUNCTION;
+  ExprBuilder &operator=(const ExprBuilder&) LLVM_DELETED_FUNCTION;
+
+protected:
+  static Expr *assertNotNull(Expr *E) {
+    assert(E && "Expression construction must not fail.");
+    return E;
+  }
+
+public:
+  ExprBuilder() {}
+  virtual ~ExprBuilder() {}
+
+  virtual Expr *build(Sema &S, SourceLocation Loc) const = 0;
+};
+
+class RefBuilder: public ExprBuilder {
+  VarDecl *Var;
+  QualType VarType;
+
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return assertNotNull(S.BuildDeclRefExpr(Var, VarType, VK_LValue, Loc).take());
+  }
+
+  RefBuilder(VarDecl *Var, QualType VarType)
+      : Var(Var), VarType(VarType) {}
+};
+
+class ThisBuilder: public ExprBuilder {
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return assertNotNull(S.ActOnCXXThis(Loc).takeAs<Expr>());
+  }
+};
+
+class CastBuilder: public ExprBuilder {
+  const ExprBuilder &Builder;
+  QualType Type;
+  ExprValueKind Kind;
+  const CXXCastPath &Path;
+
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return assertNotNull(S.ImpCastExprToType(Builder.build(S, Loc), Type,
+                                             CK_UncheckedDerivedToBase, Kind,
+                                             &Path).take());
+  }
+
+  CastBuilder(const ExprBuilder &Builder, QualType Type, ExprValueKind Kind,
+              const CXXCastPath &Path)
+      : Builder(Builder), Type(Type), Kind(Kind), Path(Path) {}
+};
+
+class DerefBuilder: public ExprBuilder {
+  const ExprBuilder &Builder;
+
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return assertNotNull(
+        S.CreateBuiltinUnaryOp(Loc, UO_Deref, Builder.build(S, Loc)).take());
+  }
+
+  DerefBuilder(const ExprBuilder &Builder) : Builder(Builder) {}
+};
+
+class MemberBuilder: public ExprBuilder {
+  const ExprBuilder &Builder;
+  QualType Type;
+  CXXScopeSpec SS;
+  bool IsArrow;
+  LookupResult &MemberLookup;
+
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return assertNotNull(S.BuildMemberReferenceExpr(
+        Builder.build(S, Loc), Type, Loc, IsArrow, SS, SourceLocation(), 0,
+        MemberLookup, 0).take());
+  }
+
+  MemberBuilder(const ExprBuilder &Builder, QualType Type, bool IsArrow,
+                LookupResult &MemberLookup)
+      : Builder(Builder), Type(Type), IsArrow(IsArrow),
+        MemberLookup(MemberLookup) {}
+};
+
+class MoveCastBuilder: public ExprBuilder {
+  const ExprBuilder &Builder;
+
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return assertNotNull(CastForMoving(S, Builder.build(S, Loc)));
+  }
+
+  MoveCastBuilder(const ExprBuilder &Builder) : Builder(Builder) {}
+};
+
+class LvalueConvBuilder: public ExprBuilder {
+  const ExprBuilder &Builder;
+
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return assertNotNull(
+        S.DefaultLvalueConversion(Builder.build(S, Loc)).take());
+  }
+
+  LvalueConvBuilder(const ExprBuilder &Builder) : Builder(Builder) {}
+};
+
+class SubscriptBuilder: public ExprBuilder {
+  const ExprBuilder &Base;
+  const ExprBuilder &Index;
+
+public:
+  virtual Expr *build(Sema &S, SourceLocation Loc) const
+      LLVM_OVERRIDE {
+    return assertNotNull(S.CreateBuiltinArraySubscriptExpr(
+        Base.build(S, Loc), Loc, Index.build(S, Loc), Loc).take());
+  }
+
+  SubscriptBuilder(const ExprBuilder &Base, const ExprBuilder &Index)
+      : Base(Base), Index(Index) {}
+};
+
+} // end anonymous namespace
+
 /// When generating a defaulted copy or move assignment operator, if a field
 /// should be copied with __builtin_memcpy rather than via explicit assignments,
 /// do so. This optimization only applies for arrays of scalars, and for arrays
 /// of class type where the selected copy/move-assignment operator is trivial.
 static StmtResult
 buildMemcpyForAssignmentOp(Sema &S, SourceLocation Loc, QualType T,
-                           Expr *To, Expr *From) {
+                           const ExprBuilder &ToB, const ExprBuilder &FromB) {
   // Compute the size of the memory buffer to be copied.
   QualType SizeType = S.Context.getSizeType();
   llvm::APInt Size(S.Context.getTypeSize(SizeType),
                    S.Context.getTypeSizeInChars(T).getQuantity());
 
   // Take the address of the field references for "from" and "to". We
   // directly construct UnaryOperators here because semantic analysis
   // does not permit us to take the address of an xvalue.
+  Expr *From = FromB.build(S, Loc);
   From = new (S.Context) UnaryOperator(From, UO_AddrOf,
                          S.Context.getPointerType(From->getType()),
                          VK_RValue, OK_Ordinary, Loc);
+  Expr *To = ToB.build(S, Loc);
   To = new (S.Context) UnaryOperator(To, UO_AddrOf,
                        S.Context.getPointerType(To->getType()),
                        VK_RValue, OK_Ordinary, Loc);
 
   const Type *E = T->getBaseElementTypeUnsafe();
   bool NeedsCollectableMemCpy =
     E->isRecordType() && E->getAs<RecordType>()->getDecl()->hasObjectMember();
 

 /// \param Copying Whether we're copying or moving.
 ///
 /// \param Depth Internal parameter recording the depth of the recursion.
 ///
 /// \returns A statement or a loop that copies the expressions, or StmtResult(0)
 /// if a memcpy should be used instead.
 static StmtResult
 buildSingleCopyAssignRecursively(Sema &S, SourceLocation Loc, QualType T,
-                                 Expr *To, Expr *From,
+                                 const ExprBuilder &To, const ExprBuilder &From,
                                  bool CopyingBaseSubobject, bool Copying,
                                  unsigned Depth = 0) {
   // C++11 [class.copy]p28:
   //   Each subobject is assigned in the manner appropriate to its type:
   //
   //     - if the subobject is of class type, as if by a call to operator= with
   //       the subobject as the object expression and the corresponding
   //       subobject of x as a single function argument (as if by explicit
if (const RecordType *RecordTy = T->getAs<RecordType>()) {
     const Type *CanonicalT = S.Context.getCanonicalType(T.getTypePtr());
     SS.MakeTrivial(S.Context,
                    NestedNameSpecifier::Create(S.Context, 0, false,
                                                CanonicalT),
                    Loc);
 
     // Create the reference to operator=.
     ExprResult OpEqualRef
-      = S.BuildMemberReferenceExpr(To, T, Loc, /*isArrow=*/false, SS,
-                                   /*TemplateKWLoc=*/SourceLocation(),
+      = S.BuildMemberReferenceExpr(To.build(S, Loc), T, Loc, /*isArrow=*/false,
+                                   SS, /*TemplateKWLoc=*/SourceLocation(),
                                    /*FirstQualifierInScope=*/0,
                                    OpLookup,
                                    /*TemplateArgs=*/0,
                                    /*SuppressQualifierCheck=*/true);
     if (OpEqualRef.isInvalid())
       return StmtError();
 
     // Build the call to the assignment operator.
 
+    Expr *FromInst = From.build(S, Loc);
     ExprResult Call = S.BuildCallToMemberFunction(/*Scope=*/0,
                                                   OpEqualRef.takeAs<Expr>(),
-                                                  Loc, From, Loc);
+                                                  Loc, FromInst, Loc);
     if (Call.isInvalid())
       return StmtError();
 
     // If we built a call to a trivial 'operator=' while copying an array,
     // bail out. We'll replace the whole shebang with a memcpy.
     CXXMemberCallExpr *CE = dyn_cast<CXXMemberCallExpr>(Call.get());
     if (CE && CE->getMethodDecl()->isTrivial() && Depth)
       return StmtResult((Stmt*)0);
 
     // Convert to an expression-statement, and clean up any produced
     // temporaries.
     return S.ActOnExprStmt(Call);
   }
 
   //     - if the subobject is of scalar type, the built-in assignment
   //       operator is used.
   const ConstantArrayType *ArrayTy = S.Context.getAsConstantArrayType(T);
   if (!ArrayTy) {
-    ExprResult Assignment = S.CreateBuiltinBinOp(Loc, BO_Assign, To, From);
+    ExprResult Assignment = S.CreateBuiltinBinOp(
+        Loc, BO_Assign, To.build(S, Loc), From.build(S, Loc));
     if (Assignment.isInvalid())
       return StmtError();
     return S.ActOnExprStmt(Assignment);
   }
 
   //     - if the subobject is an array, each element is assigned, in the
   //       manner appropriate to the element type;
 
VarDecl *IterationVar = VarDecl::Create(S.Context, S.CurContext, Loc, Loc,
                                           IterationVarName, SizeType,
                             S.Context.getTrivialTypeSourceInfo(SizeType, Loc),
                                           SC_None);
 
   // Initialize the iteration variable to zero.
   llvm::APInt Zero(S.Context.getTypeSize(SizeType), 0);
   IterationVar->setInit(IntegerLiteral::Create(S.Context, Zero, SizeType, Loc));
 
-  // Create a reference to the iteration variable; we'll use this several
-  // times throughout.
-  Expr *IterationVarRef
-    = S.BuildDeclRefExpr(IterationVar, SizeType, VK_LValue, Loc).take();
-  assert(IterationVarRef && "Reference to invented variable cannot fail!");
-  Expr *IterationVarRefRVal = S.DefaultLvalueConversion(IterationVarRef).take();
-  assert(IterationVarRefRVal && "Conversion of invented variable cannot fail!");
+  // Creates a reference to the iteration variable.
+  RefBuilder IterationVarRef(IterationVar, SizeType);
+  LvalueConvBuilder IterationVarRefRVal(IterationVarRef);
 
   // Create the DeclStmt that holds the iteration variable.
   Stmt *InitStmt = new (S.Context) DeclStmt(DeclGroupRef(IterationVar),Loc,Loc);
 
   // Subscript the "from" and "to" expressions with the iteration variable.
-  From = AssertSuccess(S.CreateBuiltinArraySubscriptExpr(From, Loc,
-                                                         IterationVarRefRVal,
-                                                         Loc));
-  To = AssertSuccess(S.CreateBuiltinArraySubscriptExpr(To, Loc,
-                                                       IterationVarRefRVal,
-                                                       Loc));
-  if (!Copying) // Cast to rvalue
-    From = CastForMoving(S, From);
+  SubscriptBuilder FromIndexCopy(From, IterationVarRefRVal);
+  MoveCastBuilder FromIndexMove(FromIndexCopy);
+  const ExprBuilder *FromIndex;
+  if (Copying)
+    FromIndex = &FromIndexCopy;
+  else
+    FromIndex = &FromIndexMove;
+
+  SubscriptBuilder ToIndex(To, IterationVarRefRVal);
 
   // Build the copy/move for an individual element of the array.
   StmtResult Copy =
     buildSingleCopyAssignRecursively(S, Loc, ArrayTy->getElementType(),
-                                     To, From, CopyingBaseSubobject,
+                                     ToIndex, *FromIndex, CopyingBaseSubobject,
                                      Copying, Depth + 1);
   // Bail out if copying fails or if we determined that we should use memcpy.
   if (Copy.isInvalid() || !Copy.get())
     return Copy;
 
   // Create the comparison against the array bound.
   llvm::APInt Upper
     = ArrayTy->getSize().zextOrTrunc(S.Context.getTypeSize(SizeType));
   Expr *Comparison
-    = new (S.Context) BinaryOperator(IterationVarRefRVal,
+    = new (S.Context) BinaryOperator(IterationVarRefRVal.build(S, Loc),
                      IntegerLiteral::Create(S.Context, Upper, SizeType, Loc),
                                      BO_NE, S.Context.BoolTy,
                                      VK_RValue, OK_Ordinary, Loc, false);
 
   // Create the pre-increment of the iteration variable.
   Expr *Increment
-    = new (S.Context) UnaryOperator(IterationVarRef, UO_PreInc, SizeType,
-                                    VK_LValue, OK_Ordinary, Loc);
+    = new (S.Context) UnaryOperator(IterationVarRef.build(S, Loc), UO_PreInc,
+                                    SizeType, VK_LValue, OK_Ordinary, Loc);
 
   // Construct the loop that copies all elements of this array.
   return S.ActOnForStmt(Loc, Loc, InitStmt, 
                         S.MakeFullExpr(Comparison),
                         0, S.MakeFullDiscardedValueExpr(Increment),
                         Loc, Copy.take());
 }
 
 static StmtResult
 buildSingleCopyAssign(Sema &S, SourceLocation Loc, QualType T,
-                      Expr *To, Expr *From,
+                      const ExprBuilder &To, const ExprBuilder &From,
                       bool CopyingBaseSubobject, bool Copying) {
   // Maybe we should use a memcpy?
   if (T->isArrayType() && !T.isConstQualified() && !T.isVolatileQualified() &&
       T.isTriviallyCopyableType(S.Context))
     return buildMemcpyForAssignmentOp(S, Loc, T, To, From);
 
   StmtResult Result(buildSingleCopyAssignRecursively(S, Loc, T, To, From,
                                                      CopyingBaseSubobject,
if (const LValueReferenceType *OtherRef
                                 = OtherRefType->getAs<LValueReferenceType>()) {
     OtherRefType = OtherRef->getPointeeType();
     OtherQuals = OtherRefType.getQualifiers();
   }
   
   // Our location for everything implicitly-generated.
   SourceLocation Loc = CopyAssignOperator->getLocation();
   
-  // Construct a reference to the "other" object. We'll be using this 
-  // throughout the generated ASTs.
-  Expr *OtherRef = BuildDeclRefExpr(Other, OtherRefType, VK_LValue, Loc).take();
-  assert(OtherRef && "Reference to parameter cannot fail!");
-  
-  // Construct the "this" pointer. We'll be using this throughout the generated
-  // ASTs.
-  Expr *This = ActOnCXXThis(Loc).takeAs<Expr>();
-  assert(This && "Reference to this cannot fail!");
+  // Builds a DeclRefExpr for the "other" object.
+  RefBuilder OtherRef(Other, OtherRefType);
+
+  // Builds the "this" pointer.
+  ThisBuilder This;
   
   // Assign base classes.
   bool Invalid = false;
   for (CXXRecordDecl::base_class_iterator Base = ClassDecl->bases_begin(),
        E = ClassDecl->bases_end(); Base != E; ++Base) {
     // Form the assignment:
     //   static_cast<Base*>(this)->Base::operator=(static_cast<Base&>(other));
     QualType BaseType = Base->getType().getUnqualifiedType();
     if (!BaseType->isRecordType()) {
       Invalid = true;
       continue;
     }
 
     CXXCastPath BasePath;
     BasePath.push_back(Base);
 
     // Construct the "from" expression, which is an implicit cast to the
     // appropriately-qualified base type.
-    Expr *From = OtherRef;
-    From = ImpCastExprToType(From, Context.getQualifiedType(BaseType, OtherQuals),
-                             CK_UncheckedDerivedToBase,
-                             VK_LValue, &BasePath).take();
+    CastBuilder From(OtherRef, Context.getQualifiedType(BaseType, OtherQuals),
+                     VK_LValue, BasePath);
 
     // Dereference "this".
-    ExprResult To = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
-    
-    // Implicitly cast "this" to the appropriately-qualified base type.
-    To = ImpCastExprToType(To.take(), 
-                           Context.getCVRQualifiedType(BaseType,
-                                     CopyAssignOperator->getTypeQualifiers()),
-                           CK_UncheckedDerivedToBase, 
-                           VK_LValue, &BasePath);
+    DerefBuilder DerefThis(This);
+    CastBuilder To(DerefThis,
+                   Context.getCVRQualifiedType(
+                       BaseType, CopyAssignOperator->getTypeQualifiers()),
+                   VK_LValue, BasePath);
 
     // Build the copy.
     StmtResult Copy = buildSingleCopyAssign(*this, Loc, BaseType,
-                                            To.get(), From,
+                                            To, From,
                                             /*CopyingBaseSubobject=*/true,
                                             /*Copying=*/true);
     if (Copy.isInvalid()) {
       Diag(CurrentLocation, diag::note_member_synthesized_at) 
         << CXXCopyAssignment << Context.getTagDeclType(ClassDecl);
       CopyAssignOperator->setInvalidDecl();
       return;
     }
for (CXXRecordDecl::field_iterator Field = ClassDecl->field_begin(),
     }
     
     // Build references to the field in the object we're copying from and to.
     CXXScopeSpec SS; // Intentionally empty
     LookupResult MemberLookup(*this, Field->getDeclName(), Loc,
                               LookupMemberName);
     MemberLookup.addDecl(*Field);
     MemberLookup.resolveKind();
-    ExprResult From = BuildMemberReferenceExpr(OtherRef, OtherRefType,
-                                               Loc, /*IsArrow=*/false,
-                                               SS, SourceLocation(), 0,
-                                               MemberLookup, 0);
-    ExprResult To = BuildMemberReferenceExpr(This, This->getType(),
-                                             Loc, /*IsArrow=*/true,
-                                             SS, SourceLocation(), 0,
-                                             MemberLookup, 0);
-    assert(!From.isInvalid() && "Implicit field reference cannot fail");
-    assert(!To.isInvalid() && "Implicit field reference cannot fail");
+
+    MemberBuilder From(OtherRef, OtherRefType, /*IsArrow=*/false, MemberLookup);
+
+    MemberBuilder To(This, getCurrentThisType(), /*IsArrow=*/true, MemberLookup);
 
     // Build the copy of this field.
     StmtResult Copy = buildSingleCopyAssign(*this, Loc, FieldType,
-                                            To.get(), From.get(),
+                                            To, From,
                                             /*CopyingBaseSubobject=*/false,
                                             /*Copying=*/true);
     if (Copy.isInvalid()) {
       Diag(CurrentLocation, diag::note_member_synthesized_at) 
         << CXXCopyAssignment << Context.getTagDeclType(ClassDecl);
       CopyAssignOperator->setInvalidDecl();
       return;
     }
     
     // Success! Record the copy.
     Statements.push_back(Copy.takeAs<Stmt>());
   }
 
   if (!Invalid) {
     // Add a "return *this;"
-    ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
+    ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
     
     StmtResult Return = ActOnReturnStmt(Loc, ThisObj.get());
     if (Return.isInvalid())
       Invalid = true;
     else {
       Statements.push_back(Return.takeAs<Stmt>());
 
       if (Trap.hasErrorOccurred()) {
void Sema::DefineImplicitMoveAssignment(SourceLocation CurrentLocation,
   QualType OtherRefType = Other->getType()->
       getAs<RValueReferenceType>()->getPointeeType();
   assert(!OtherRefType.getQualifiers() &&
          "Bad argument type of defaulted move assignment");
 
   // Our location for everything implicitly-generated.
   SourceLocation Loc = MoveAssignOperator->getLocation();
 
-  // Construct a reference to the "other" object. We'll be using this 
-  // throughout the generated ASTs.
-  Expr *OtherRef = BuildDeclRefExpr(Other, OtherRefType, VK_LValue, Loc).take();
-  assert(OtherRef && "Reference to parameter cannot fail!");
+  // Builds a reference to the "other" object.
+  RefBuilder OtherRef(Other, OtherRefType);
   // Cast to rvalue.
-  OtherRef = CastForMoving(*this, OtherRef);
+  MoveCastBuilder MoveOther(OtherRef);
 
-  // Construct the "this" pointer. We'll be using this throughout the generated
-  // ASTs.
-  Expr *This = ActOnCXXThis(Loc).takeAs<Expr>();
-  assert(This && "Reference to this cannot fail!");
+  // Builds the "this" pointer.
+  ThisBuilder This;
 
   // Assign base classes.
   bool Invalid = false;
   for (CXXRecordDecl::base_class_iterator Base = ClassDecl->bases_begin(),
        E = ClassDecl->bases_end(); Base != E; ++Base) {
     // Form the assignment:
     //   static_cast<Base*>(this)->Base::operator=(static_cast<Base&&>(other));
     QualType BaseType = Base->getType().getUnqualifiedType();
     if (!BaseType->isRecordType()) {
       Invalid = true;
       continue;
     }
 
     CXXCastPath BasePath;
     BasePath.push_back(Base);
 
     // Construct the "from" expression, which is an implicit cast to the
     // appropriately-qualified base type.
-    Expr *From = OtherRef;
-    From = ImpCastExprToType(From, BaseType, CK_UncheckedDerivedToBase,
-                             VK_XValue, &BasePath).take();
+    CastBuilder From(OtherRef, BaseType, VK_XValue, BasePath);
 
     // Dereference "this".
-    ExprResult To = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
+    DerefBuilder DerefThis(This);
 
     // Implicitly cast "this" to the appropriately-qualified base type.
-    To = ImpCastExprToType(To.take(), 
-                           Context.getCVRQualifiedType(BaseType,
-                                     MoveAssignOperator->getTypeQualifiers()),
-                           CK_UncheckedDerivedToBase, 
-                           VK_LValue, &BasePath);
+    CastBuilder To(DerefThis,
+                   Context.getCVRQualifiedType(
+                       BaseType, MoveAssignOperator->getTypeQualifiers()),
+                   VK_LValue, BasePath);
 
     // Build the move.
     StmtResult Move = buildSingleCopyAssign(*this, Loc, BaseType,
-                                            To.get(), From,
+                                            To, From,
                                             /*CopyingBaseSubobject=*/true,
                                             /*Copying=*/false);
     if (Move.isInvalid()) {
       Diag(CurrentLocation, diag::note_member_synthesized_at) 
         << CXXMoveAssignment << Context.getTagDeclType(ClassDecl);
       MoveAssignOperator->setInvalidDecl();
       return;
     }
for (CXXRecordDecl::field_iterator Field = ClassDecl->field_begin(),
     QualType FieldType = Field->getType().getNonReferenceType();
     if (FieldType->isIncompleteArrayType()) {
       assert(ClassDecl->hasFlexibleArrayMember() && 
              "Incomplete array type is not valid");
       continue;
     }
     
     // Build references to the field in the object we're copying from and to.
-    CXXScopeSpec SS; // Intentionally empty
     LookupResult MemberLookup(*this, Field->getDeclName(), Loc,
                               LookupMemberName);
     MemberLookup.addDecl(*Field);
     MemberLookup.resolveKind();
-    ExprResult From = BuildMemberReferenceExpr(OtherRef, OtherRefType,
-                                               Loc, /*IsArrow=*/false,
-                                               SS, SourceLocation(), 0,
-                                               MemberLookup, 0);
-    ExprResult To = BuildMemberReferenceExpr(This, This->getType(),
-                                             Loc, /*IsArrow=*/true,
-                                             SS, SourceLocation(), 0,
-                                             MemberLookup, 0);
-    assert(!From.isInvalid() && "Implicit field reference cannot fail");
-    assert(!To.isInvalid() && "Implicit field reference cannot fail");
+    MemberBuilder From(MoveOther, OtherRefType,
+                       /*IsArrow=*/false, MemberLookup);
+    MemberBuilder To(This, getCurrentThisType(),
+                     /*IsArrow=*/true, MemberLookup);
 
-    assert(!From.get()->isLValue() && // could be xvalue or prvalue
+    assert(!From.build(*this, Loc)->isLValue() && // could be xvalue or prvalue
         "Member reference with rvalue base must be rvalue except for reference "
         "members, which aren't allowed for move assignment.");
 
     // Build the move of this field.
     StmtResult Move = buildSingleCopyAssign(*this, Loc, FieldType,
-                                            To.get(), From.get(),
+                                            To, From,
                                             /*CopyingBaseSubobject=*/false,
                                             /*Copying=*/false);
     if (Move.isInvalid()) {
       Diag(CurrentLocation, diag::note_member_synthesized_at) 
         << CXXMoveAssignment << Context.getTagDeclType(ClassDecl);
       MoveAssignOperator->setInvalidDecl();
       return;
     }
 
     // Success! Record the copy.
     Statements.push_back(Move.takeAs<Stmt>());
   }
 
   if (!Invalid) {
     // Add a "return *this;"
-    ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This);
+    ExprResult ThisObj = CreateBuiltinUnaryOp(Loc, UO_Deref, This.build(*this, Loc));
     
     StmtResult Return = ActOnReturnStmt(Loc, ThisObj.get());
     if (Return.isInvalid())
       Invalid = true;
     else {
       Statements.push_back(Return.takeAs<Stmt>());
 
       if (Trap.hasErrorOccurred()) {

cfe/trunk/test/Analysis/operator-calls.cpp

-// RUN: %clang_cc1 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -verify %s
+// RUN: %clang_cc1 -std=c++11 -analyze -analyzer-checker=core,alpha.core,debug.ExprInspection -verify %s
 void clang_analyzer_eval(bool);
 
 struct X0 { };
 bool operator==(const X0&, const X0&);
 
 // PR7287
 struct test { int a[2]; };
 
namespace RValues {
 
   void test(int coin) {
     // Force a cache-out when we try to conjure a temporary region for the operator call.
     // ...then, don't crash.
     clang_analyzer_eval(+(coin ? getSmallOpaque() : getSmallOpaque())); // expected-warning{{UNKNOWN}}
     clang_analyzer_eval(+(coin ? getLargeOpaque() : getLargeOpaque())); // expected-warning{{UNKNOWN}}
   }
 }
+
+namespace SynthesizedAssignment {
+  struct A {
+    int a;
+    A& operator=(A& other) { a = -other.a; return *this; }
+    A& operator=(A&& other) { a = other.a+1; return *this; }
+  };
+
+  struct B {
+    int x;
+    A a[3];
+    B& operator=(B&) = default;
+    B& operator=(B&&) = default;
+  };
+
+  // This used to produce a warning about the iteration variable in the
+  // synthesized assignment operator being undefined.
+  void testNoWarning() {
+    B v, u;
+    u = v;
+  }
+
+  void testNoWarningMove() {
+    B v, u;
+    u = static_cast<B &&>(v);
+  }
+
+  void testConsistency() {
+    B v, u;
+    v.a[1].a = 47;
+    v.a[2].a = 42;
+    u = v;
+    clang_analyzer_eval(u.a[1].a == -47); // expected-warning{{TRUE}}
+    clang_analyzer_eval(u.a[2].a == -42); // expected-warning{{TRUE}}
+  }
+
+  void testConsistencyMove() {
+    B v, u;
+    v.a[1].a = 47;
+    v.a[2].a = 42;
+    u = static_cast<B &&>(v);
+    clang_analyzer_eval(u.a[1].a == 48); // expected-warning{{TRUE}}
+    clang_analyzer_eval(u.a[2].a == 43); // expected-warning{{TRUE}}
+  }
+}