kimgr added a comment.
I have now convinced myself that including `FullExpr` in
`skipImplicitTemporary` gives an improvement in `consteval` diagnostics. But
I'm still not sure why. Motivating example, derived from cxx2a-consteval.cpp:
struct A {
int *p = new int(42);
consteval A ret_a() const {
return A{};
}
};
consteval const A &to_lvalue_ref(const A &&a) {
return a;
}
void test() {
constexpr A a{nullptr};
// error: call to consteval function 'A::ret_a' is not a constant expression
// { (void)a.ret_a(); }
// error: call to consteval function 'to_lvalue_ref' is not a constant
expression
// { (void)to_lvalue_ref(A{}); }
// error: call to consteval function 'to_lvalue_ref' is not a constant
expression
// but should probably also raise
// error: call to consteval function 'A::ret_a' is not a constant expression
{ (void)to_lvalue_ref(a.ret_a()); }
}
It's interesting to experiment with these one by one
- `A::ret_a` returns a new A, whose constructor does heap allocation; no
consteval possible
- `to_lvalue_ref` attempts to return a reference to a temporary; no consteval
possible
Composing the two as in the last example, it seems to me, should print both
diagnostics. Mainline Clang doesn't, but changing `skipImplicitTemporary` to
also skip `FullExpr`s does (also allowing removal of all `IgnoreImplicit` from
`getSubExprAsWritten` and `getConversionFunction`).
It seems intuitively right to me. I'm just a little peeved that I can't figure
out the connection between the diagnostic emission and `skipImplicitTemporary`.
================
Comment at: clang/lib/AST/Expr.cpp:1946-1947
for (const CastExpr *E = this; E; E = dyn_cast<ImplicitCastExpr>(SubExpr)) {
SubExpr = skipImplicitTemporary(E->getSubExpr());
+ SubExpr = SubExpr->IgnoreImplicit();
----------------
davrec wrote:
> aaron.ballman wrote:
> > `IgnoreImplicit()` calls `IgnoreImplicitSingleStep()` eventually, and that
> > call does the same work as `skipImplicitTemporary()`, so I think the end
> > result here should be the same.
> As I look at this a second time, I just realized...calling IgnoreImplicit
> here mean that the loop only ever runs one iteration, since IgnoreImplicit
> presumably skips over ImplicitCastExprs. While I liked how Kim did revision
> initially because it seemed to handle the constructor conversions similarly
> to their handling of getSubExprAsWritten() above, now I think something
> different is needed here.
>
> Proposed alternative:
> Right now skipIimplicitTemporary does what IgnoreImplicit does *except* skip
> over a) ImplicitCastExprs and b) FullExprs (= ConstantExprs or
> ExprWithCleanups).
>
> Kim has identified that we need to skip over at least ConstantExprs at least
> in this case (i.e. the most conservative possible fix would be to skip over
> ConstantExprs just before the cast in line 1950).
>
> But perhaps the better solution, to forestall future bugs, is to skip over
> FullExprs in skipImplicitTemporary, so that it skips over everything
> IgnoreImplicit does except ImplicitCastExprs. (And, some documentation
> should be added to `skipImplicitTemporary` to that effect, to aid future
> maintenance.)
>
> I can't see offhand how the other uses of skipImplicitTemporary would be
> negatively affected by additionally skipping over FullExprs.
>
> Aaron what do you think? Kim can you verify this alternative would also
> solve the problem without breaking any tests?
>
@aaron.ballman Just removing the `skipImplicitTemporary` line is probably not
going to work, since that's the first time `SubExpr` is assigned a
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D117391/new/
https://reviews.llvm.org/D117391
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
