[PATCH] D53475: Create ConstantExpr class

[Richard Smith - zygoloid via Phabricator via cfe-commits]

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