[PATCH] D54355: Use is.constant intrinsic for __builtin_constant_p

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

rsmith added inline comments.


================
Comment at: include/clang/AST/Expr.h:908-912
+  static ConstantExpr *Create(const ASTContext &Context, Expr *E) {
+    if (ConstantExpr *CE = dyn_cast<ConstantExpr>(E))
+      return CE;
+    return new (Context) ConstantExpr(E);
+  }
----------------
If we're creating two `ConstantExpr` wrappers for the same expression, that seems like a bug in the caller to me. When does this happen?


================
Comment at: lib/AST/Expr.cpp:2915-2916
+  case ConstantExprClass: {
+    const Expr *Exp = cast<ConstantExpr>(this)->getSubExpr();
+    return Exp->isConstantInitializer(Ctx, false, Culprit);
+  }
----------------
Can we just return `true` here? If not, please add a FIXME saying that we should be able to.


================
Comment at: lib/Sema/SemaExpr.cpp:4973-4974
 
+    if ((ICEArguments & (1 << (ArgIx - 1))) != 0)
+      Arg = ConstantExpr::Create(Context, Arg);
+
----------------
We should create this node as part of checking that the argument is an ICE (in `SemaBuiltinConstantArg`).


================
Comment at: lib/Sema/SemaExpr.cpp:14156-14157
 
+  if (!CurContext->isFunctionOrMethod())
+    E = ConstantExpr::Create(Context, E);
+
----------------
I don't understand why `CurContext` matters here. Can you explain the intent of this check?


Repository:
  rC Clang

https://reviews.llvm.org/D54355