aaron.ballman added a comment.
In D117391#3299483 <https://reviews.llvm.org/D117391#3299483>, @kimgr wrote:
> 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
Agreed. This stuff is about as clear as mud given that the rules change in
every version of C++ and no two compilers implement the same constant
expression evaluation rules at this point.
> - `A::ret_a` returns a new A, whose constructor does heap allocation; no
> consteval possible
Agreed, though there are cases where it could be possible:
http://eel.is/c++draft/expr.const#5.19.
> - `to_lvalue_ref` attempts to return a reference to a temporary; no consteval
> possible
Hmm, is that still the case? http://eel.is/c++draft/expr.const#4.7
> 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`.
Assuming that the `to_lvalue_ref(A{})` case should diagnose, then yes, I agree,
I think the `ret_a()` portion should as well.
================
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();
----------------
kimgr wrote:
> 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
> Aaron what do you think? Kim can you verify this alternative would also solve
> the problem without breaking any tests?
I think that's a good idea to explore. I hadn't spotted the `FullExpr`
difference when I was looking at it before.
================
Comment at: clang/lib/AST/Expr.cpp:1949-1950
if (E->getCastKind() == CK_ConstructorConversion)
return cast<CXXConstructExpr>(SubExpr)->getConstructor();
----------------
kimgr wrote:
> kimgr wrote:
> > davrec wrote:
> > > I think the errors prove we should fall back to the most conservative
> > > possible fix: remove all the other changes and just change these 2 lines
> > > to:
> > > ```
> > > if (E->getCastKind() == CK_ConstructorConversion) {
> > > if (auto *CE = dyn_cast<ConstantExpr>(SubExpr)
> > > SubExpr = CE->getSubExpr();
> > > return cast<CXXConstructExpr>(SubExpr)->getConstructor();
> > > }
> > > ```
> > > I'm can't quite remember but I believe that would solve the bug as
> > > narrowly as possible. @kimgr can you give it a try if and see if it
> > > avoids the errors?
> > > (If it doesn't, I'm stumped...)
> > Yes, it does. I needed to add the same `ConstantExpr` skipping to the
> > user-defined conversion handling below, but once those two were in place
> > the new unittests succeed and the existing lit tests also.
> >
> > It does feel a little awkward, though... I wish I had a clearer mental
> > model of how the implicit-skipping is supposed to work. My intuition is
> > telling me `skipImplicitTemporary` should probably disappear and we should
> > use the `IgnoreExpr.h` facilities directly instead, but it's all a little
> > fuzzy to me at this point.
> >
> > I'll run the full `check-clang` suite overnight with this alternative
> > patch, I have no reason to think there will be problems, but I'll make sure
> > in the morning.
> >
> > Thanks!
> I can confirm that full `check-clang` also works with the narrower fix.
> My intuition is telling me skipImplicitTemporary should probably disappear
> and we should use the IgnoreExpr.h facilities directly instead, but it's all
> a little fuzzy to me at this point.
This matches the direction I think we should be heading. If we need to add a
new facility to `IgnoreExpr.h`, that's also fine, but comments describing how
it differs from the existing facilities would be critical.
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
