Izaron added a comment. In D119646#3317473 <https://reviews.llvm.org/D119646#3317473>, @cor3ntin wrote:
> There is also https://reviews.llvm.org/D74130 which tries to address the same > issue in a very different way. > I don't really understand the different approaches yet. Thanks! I spent some time researching this review (but tbh I don't fully understand it yet too). I can say why this patch seems to be so different. The author aims to make this snippet working consteval int f1() { return 0; } consteval auto g() { return f1; } consteval int h(int (*p)() = g()) { return p(); } // should work That is, they want to make usable pointers to consteval function in default arguments. My patch doesn't compile this code now (the behaviour is same as in trunk). As far as I see, the main problem is that the evaluation of ParmVarDecl's ConstantExpr is "isolated" from the evaluation of the consteval function body. Therefore the pointer "breaks the constexpr wall" and "falls out" to run-time, that's illegal. I'm not sure if my explanation is clear... I wonder if it could be a different pull request later if someone will want to make pointers working? The review you mentioned is 2 years old, pretty massive and still has failing tests. ================ Comment at: clang/lib/AST/Decl.cpp:2816 if (auto *E = dyn_cast_or_null<FullExpr>(Arg)) - return E->getSubExpr(); + if (!isa<ConstantExpr>(E)) + return E->getSubExpr(); ---------------- erichkeane wrote: > So what is happening here? I would still expect us to give the proper > default-arg in this case? What subtly am I missing? If we have nested ConstantExprs within the default argument expression, like here ``` consteval int Fun(int x) { return x; } struct Test { Test(int loc = Fun(1) * 3 + Fun(2)) {} // or, for example, Test(int loc = 0 + Fun(4)); }; ``` Then the patch fixes it fine with just `void VisitConstantExpr...` (the snippet where you said //"This part makes sense to me."//). The AST for ParmVarDecl looks like this - [[ https://gist.githubusercontent.com/Izaron/7b49d4814a04d16c8014d973bd397ac4/raw/fd5948ca164a381ed13950f04934cb19f847141d/nested.ast | snippet on gist.github.com ]]. Clang will fold the ConstantExprs. --- However, if the ConstantExpr is top-level, that is, the code looks like this: ``` struct Test { Test(int loc = Fun(4)) {} }; ``` Then the AST looks like this - [[ https://gist.githubusercontent.com/Izaron/dec1f3f0b62769c8fe4f4cb961c211d7/raw/79c5b9364e93de7ebc3d905f87162e000c6e695b/top-level.ast | snippet on gist.github.com ]]. There are some places in the code where we call `ParmVarDecl::getDefaultArg()` (CodeGen/etc.) The callee will "jump over" the ConstantExpr (since it is derived from FullExpr) and give you the nested CallExpr. That's quite incorrent because we aren't going to evaluate this CallExpr in run-time. For example, if we don't patch this place, the CodeGen will declare `Fun` at LLVM IR, and everything breaks. So I decided that we shouldn't "jump over" ConstantExpr, otherwise it incorrectly says to the caller that we are supposed to calculate it in run-time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119646/new/ https://reviews.llvm.org/D119646 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
