[PATCH] Sema: avoid reuse of Exprs when synthesizing operator=

[Pavel Labath]

================
Comment at: lib/Sema/SemaDeclCXX.cpp:8493
@@ +8492,3 @@
+//  avoid using the same Expr* in the AST twice.
+class ExprBuilder {
+  ExprBuilder(const ExprBuilder&) LLVM_DELETED_FUNCTION;
----------------
Manuel Klimek wrote:
> 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 ;)
Ok, I'm leaving the members in the present order for now.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:8505
@@ +8504,3 @@
+    Expr *E = DoBuild(S, Loc);
+    assert(E && "Expression construction cannot fail!");
+    return E;
----------------
Manuel Klimek wrote:
> 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.
I've put the assertion into a helper method.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:8514-8518
@@ +8513,7 @@
+protected:
+  virtual Expr *DoBuild(Sema &S, SourceLocation Loc, Expr *E) const = 0;
+
+  virtual Expr *DoBuild(Sema &S, SourceLocation Loc) const LLVM_OVERRIDE {
+    return DoBuild(S, Loc, Builder.Build(S, Loc));
+  }
+
----------------
Manuel Klimek wrote:
> 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.
I've removed this intermediate class.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:8867
@@ -8721,9 +8866,3 @@
   // 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 FromIdx(From, IterationVarRefRVal);
+  SubscriptBuilder ToIdx(To, IterationVarRefRVal);
----------------
Manuel Klimek wrote:
> I'd s/Idx/Index/
Usage of "Index" seems to be more common in this file, replaced.

================
Comment at: lib/Sema/SemaDeclCXX.cpp:8871
@@ -8730,1 +8870,3 @@
+      Copying ? FromIdx
+              : static_cast<const ExprBuilder &>(MoveCastBuilder(FromIdx));
 
----------------
Manuel Klimek wrote:
> That cast seems wrong. Please try to reformulate without the cast. Use pointers if necessary.
Reformulated.


http://llvm-reviews.chandlerc.com/D1425