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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits