[PATCH] D151587: [clang][ConstantEmitter] have tryEmitPrivate[ForVarInit] try ConstExprEmitter fast-path first

[Nick Desaulniers via Phabricator via cfe-commits]

nickdesaulniers added a comment.

In D151587#4516772 <https://reviews.llvm.org/D151587#4516772>, @efriedma wrote:

> This seems to work:
>
>   diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
>   index 25d3535e..98b1e4d 100644
>   --- a/clang/lib/AST/Expr.cpp
>   +++ b/clang/lib/AST/Expr.cpp
>   @@ -3319,6 +3319,10 @@ bool Expr::isConstantInitializer(ASTContext &Ctx, bool IsForRef,
>      // kill the second parameter.
>   
>      if (IsForRef) {
>   +    if (auto *EWC = dyn_cast<ExprWithCleanups>(this))
>   +      return EWC->getSubExpr()->isConstantInitializer(Ctx, true, Culprit);
>   +    if (auto *MTE = dyn_cast<MaterializeTemporaryExpr>(this))
>   +      return MTE->getSubExpr()->isConstantInitializer(Ctx, false, Culprit);

Indeed, but I wonder why only do this when `IsForRef == true` (narrator: it's not)? Also, why for `MaterializeTemporaryExpr` manually force the `ForRef` parameter to `false`?

---

It seems like perhaps we want to call `EvaluateAsLValue` in `Expr::isConstantInitializer` AFTER checking the statement class.  Otherwise we're basically reimplementing that big switch statement again; at least two cases as suggested by your diff.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151587/new/

https://reviews.llvm.org/D151587