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

Mon Jun 5 15:30:34 PDT 2023

nickdesaulniers added a comment.

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

> Two points I'm not sure about in the current version:
>
> - Handling MaterializeTemporaryExpr in ConstExprEmitter doesn't make sense; ConstExprEmitter is not supposed to visit lvalues. (And I'm not sure what the new check is supposed to do; `E->isGLValue()` is always true for a MaterializeTemporaryExpr.)  Maybe try something like the following?

Yeah, that works.

> - HasAnyMaterializeTemporaryExpr shouldn't need to handle VisitCXXConstructExpr.  The EvaluateAsLValue bug only applies to lifetime-extended temporaries.  Any MaterializeTemporaryExpr that's the operand of a VisitCXXConstructExpr should not be lifetime-extended beyond the end of the expression. So it won't run into the EvaluateAsLValue issue.  (https://en.cppreference.com/w/cpp/language/reference_initialization#Lifetime_of_a_temporary should be a complete list of cases where we might need to recurse; none of those corresponds to a CXXConstructExpr.)

See the AST in https://reviews.llvm.org/D151587#4396705; without a visitor for that, we don't recurse far enough down to see there is a MaterializeTemporaryExpr child in the sub-AST.  Maybe that's because of my subclassing of `ConstStmtVisitor` returning `bool`, so if I don't implement a visitor, the default visit returns `false` (if I understand CRTP/Visitor-pattern correctly).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151587