================
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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
