[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr closed this revision. kimgr added a comment. Herald added a project: All. Now that D119477 has landed, this suggested change is obsolete. Closing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ h

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

aaron.ballman added a comment. In D117391#3301696 , @kimgr wrote: > @aaron.ballman Thanks for the valuable feedback! > >> Assuming that the `to_lvalue_ref(A{})` case should diagnose, then yes, I >> agree, I think the `ret_a()` portion should as well. > >

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @aaron.ballman Thanks for the valuable feedback! > Assuming that the `to_lvalue_ref(A{})` case should diagnose, then yes, I > agree, I think the `ret_a()` portion should as well. My primary focus is not to make sure Clang follows the standard closely for `consteval`, but

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

aaron.ballman added a comment. In 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, der

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added inline comments. Comment at: clang/lib/AST/Expr.cpp:1949-1950 if (E->getCastKind() == CK_ConstructorConversion) return cast(SubExpr)->getConstructor(); kimgr wrote: > davrec wrote: > > I think the errors prove we should fall back to th

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added inline comments. Comment at: clang/lib/AST/Expr.cpp:1949-1950 if (E->getCastKind() == CK_ConstructorConversion) return cast(SubExpr)->getConstructor(); davrec wrote: > I think the errors prove we should fall back to the most conservati

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

davrec added inline comments. Comment at: clang/lib/AST/Expr.cpp:1949-1950 if (E->getCastKind() == CK_ConstructorConversion) return cast(SubExpr)->getConstructor(); I think the errors prove we should fall back to the most conservative possible fi

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. Here's the diff between my patch (main) and handling `FullExpr` in `skipImplicitTemporary` as in the diff I posted above (patch.txt): --- main.txt 2022-02-02 20:37:21.619534225 +0100 +++ patch.txt 2022-02-02 20:34:17.016949227 +0100 @@ -192,6 +192,13 @@ /home/kim

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. Alright, with this diff: diff --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp index d502b3f1e93e..1716ac0be7ef 100644 --- a/clang/lib/AST/Expr.cpp +++ b/clang/lib/AST/Expr.cpp @@ -1908,6 +1908,9 @@ namespace { if (auto *Binder = dyn_cast(E))

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @aaron.ballman @davrec Thank you both! I started with @davrec's suggestion from https://github.com/llvm/llvm-project/issues/53044#issuecomment-1006894536, and while it fixed the problem it broke several other consteval lit tests. It could be that those test breaks are be

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

davrec added inline comments. Comment at: clang/lib/AST/Expr.cpp:1946-1947 for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) { SubExpr = skipImplicitTemporary(E->getSubExpr()); +SubExpr = SubExpr->IgnoreImplicit(); aaron.ballman wrote: > `Ign

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

aaron.ballman added a reviewer: aaron.ballman. aaron.ballman added a comment. I think the changes here look reasonable, though I think it can be simplified a bit. Comment at: clang/lib/AST/Expr.cpp:1946-1947 for (const CastExpr *E = this; E; E = dyn_cast(SubExpr)) { Su

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @rsmith Gentle ping 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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. @rsmith Weekly ping 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

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. - Update broken test case - Rebase on latest main (8eb74626f ) - Build and run `ninja check-clang` I don't have commit access, so I would appreciate help landing. Let me know if there's anything else I c

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr updated this revision to Diff 400381. kimgr added a comment. Fix spurious semicolon Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391 Files: clang/lib/AST/Expr.cpp clang/unittests/Tooling/CastExpr

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added inline comments. Comment at: clang/unittests/Tooling/CastExprTest.cpp:111 + "const char *f() {\n" + " constexpr X x;\n"; + " return x;\n" Oops, spurious semicolon here. Will post a new version once

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

davrec accepted this revision. davrec added a comment. This revision is now accepted and ready to land. Looks good, thanks for fixing this! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr added a comment. Oh yeah, this closes bug https://github.com/llvm/llvm-project/issues/53044. Do I mention that in the commit message/patch summary? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117391/new/ https://reviews.llvm.org/D117391 _

[PATCH] D117391: [AST] Ignore implicit nodes in CastExpr::getConversionFunction

kimgr created this revision. kimgr requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Under -std=c++20 with use of consteval, the subexpression hierarchy sometimes contains ConstantExpr nodes that getConversionFunction did not expect. CastExp