rsmith added a comment.
Thanks, this is looking good.
================
Comment at: include/clang/AST/Expr.h:874-875
+
+/// FullExpression - Represents a "full-expression" node.
+class FullExpression : public Expr {
+protected:
----------------
All of our expression classes have names ending `Expr`; I don't think we should
deviate from that convention here.
================
Comment at: include/clang/AST/Expr.h:886-889
+ virtual const Expr *getSubExpr() const = 0;
+ virtual Expr *getSubExpr() = 0;
+
+ virtual unsigned getNumObjects() const { return 0; }
----------------
These should not be `virtual`; this class hierarchy uses LLVM-style RTTI rather
than vtables. If you want to provide these convenience methods on `FullExpr`,
you can manually dispatch on the dynamic type using `dyn_cast` or similar.
================
Comment at: include/clang/AST/Expr.h:924-925
+ // Iterators
+ child_range children() { return Val->children(); }
+ const_child_range children() const { return Val->children(); }
+};
----------------
The children range for a `ConstantExpr` should comprise `Val` itself, not
`Val`'s children.
================
Comment at: include/clang/Basic/StmtNodes.td:97
+// Wrapper expressions
+def ConstantExpr : DStmt<Expr>;
+
----------------
Please add a `def FullExpr : DStmt<Expr, 1>` and change this and
`ExprWithCleanups` to be `: DStmt<FullExpr>` so that our visitors will be able
to visit the `FullExpr` base class.
================
Comment at: lib/AST/Expr.cpp:3100-3101
+ case ConstantExprClass:
+ return cast<ConstantExpr>(this)->getSubExpr()->HasSideEffects(
+ Ctx, IncludePossibleEffects);
+
----------------
Please add a FIXME here to move this into the `return false;` block. (Keeping
this functionally identical to an `ExprWithCleanups` with no cleanups for now
seems fine.)
Repository:
rC Clang
https://reviews.llvm.org/D53475
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
