ilya-biryukov added a comment.
In D153294#4437381 <https://reviews.llvm.org/D153294#4437381>, @Fznamznon wrote:
> In fact, I can't come up with the test that this patch would break. Either
> `ExprWithCleanups` is redundant or added by different parts of Sema.
Same here, maybe @rsmith can come up with something that breaks.
The AST does seem off for this example:
struct A {
int *p = new int(42);
consteval int get() const {
return *p;
}
constexpr ~A() {
delete p;
}
};
int k = A().get() + 1;
With the patch we get `ExprWithCleanups` under `VarDecl`:
`-VarDecl 0x56375b44f7b0 <line:12:1, col:21> col:5 k 'int' cinit
`-ExprWithCleanups 0x56375b44fef0 <col:9, col:21> 'int'
`-BinaryOperator 0x56375b44fed0 <col:9, col:21> 'int' '+'
|-ConstantExpr 0x56375b44fe90 <col:9, col:17> 'int'
| `-CXXMemberCallExpr 0x56375b44fe58 <col:9, col:17> 'int'
| `-MemberExpr 0x56375b44fe28 <col:9, col:13> '<bound member function
type>' .get 0x56375b42f028
| `-ImplicitCastExpr 0x56375b44fe78 <col:9, col:11> 'const A'
xvalue <NoOp>
| `-MaterializeTemporaryExpr 0x56375b44fe10 <col:9, col:11>
'A':'A' xvalue
| `-CXXBindTemporaryExpr 0x56375b44fdf0 <col:9, col:11> 'A':'A'
(CXXTemporary 0x56375b44fdf0)
| `-CXXTemporaryObjectExpr 0x56375b44fdb8 <col:9, col:11>
'A':'A' 'void () noexcept(false)' zeroing
`-IntegerLiteral 0x56375b44feb0 <col:21> 'int' 1
But per Richard's comments `ConstantExpr` is a full expression and the cleanups
should be under it instead (see godbolt <https://gcc.godbolt.org/z/shazM5qez>
for what's happening today).
From playing around with it, I couldn't find anything where this affects either
codegen or diagnostics.
I think that's because any temporaries produced must have constexpr or trivial
destructors, so they don't produce any code. Additionally, there seems to be no
way for those temporaries to escape the immediate invocation.
I would guess we are still not comfortable with moving `ExprWithCleanups`
outside `ConstantExpr`, but let's wait for Richard's comments.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D153294/new/
https://reviews.llvm.org/D153294
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
