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