================
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;
----------------
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 ;)
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8505
@@ +8504,3 @@
+ Expr *E = DoBuild(S, Loc);
+ assert(E && "Expression construction cannot fail!");
+ return E;
----------------
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.
================
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));
+ }
+
----------------
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.
================
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);
----------------
I'd s/Idx/Index/
================
Comment at: lib/Sema/SemaDeclCXX.cpp:8871
@@ -8730,1 +8870,3 @@
+ Copying ? FromIdx
+ : static_cast<const ExprBuilder &>(MoveCastBuilder(FromIdx));
----------------
That cast seems wrong. Please try to reformulate without the cast. Use pointers
if necessary.
http://llvm-reviews.chandlerc.com/D1425
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
