Details
- Reviewers
shafik aaron.ballman Fznamznon erichkeane - Group Reviewers
Restricted Project - Commits
- rG467688527017: [clang] Implement P2564 "consteval must propagate up"
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/AST/Decl.cpp | ||
---|---|---|
3199 | This code doesn't seem to match the comment (it matches the code below though). | |
3203–3205 | ||
clang/lib/Sema/SemaDeclCXX.cpp | ||
2453 | ||
clang/lib/Sema/SemaExpr.cpp | ||
17991 | Ouch, this is pretty expensive because it actually evaluates the constant expression (and promptly forgets what it evaluated). | |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
5028–5030 |
clang/include/clang/AST/Decl.h | ||
---|---|---|
2391 | Some documentation on the new API would be useful; seems like most calls to isConsteval() should be using isImmediateFunction() instead? |
- Address Aaron's comments
- Track which expressions are immediately escalating to offer better diagnostics
In FunctionScopeInfo, only track that an immediate-escalating expression
has been found rather than which one - as that is recorded on
the expression itself.
clang/lib/Parse/ParseDecl.cpp | ||
---|---|---|
2495 | What is the point for an additional expression evaluation context here? | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
2477 | ||
clang/lib/Sema/SemaExpr.cpp | ||
17960 | CallExpr::getCallee() can return nullptr for example for indirect calls. Probably makes sense to check that it is not nullptr before accessing it. | |
17966 | Same is actually for getCurFunction(). Can be a nullptr if there is no function. | |
18202–18205 | ||
18204 | ||
clang/test/SemaCXX/cxx2a-consteval-default-params.cpp | ||
1 | Should the new behavior only be enabled for c++23? | |
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp | ||
2 | New line is not needed here, I guess. | |
9 | These examples exactly match the ones provided by P2564R3, should they be in a separate test in CXX directory then? |
clang/include/clang/AST/Decl.h | ||
---|---|---|
2391 | Yes - I added a comment | |
clang/lib/AST/Decl.cpp | ||
3180 | I don't think so - "defaulted" usually does not imply explicit, unless explicitly stated. the case would be a member init with an immediately escalating expression | |
clang/lib/Parse/ParseDecl.cpp | ||
2495 | Unnecessary, good catch! I think i had to add that in an earlier iteration of the patch which checked for immediate escalation in MarkDeclRefReferenced. | |
clang/lib/Sema/SemaExpr.cpp | ||
17966 | There is an assert above , this function should only be called from a function scope | |
clang/test/SemaCXX/cxx2a-consteval-default-params.cpp | ||
1 | Nah, it was voted as a DR |
Out of curiosity - does something like these examples - https://godbolt.org/z/Eqb58Wqoo work as expected?
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp | ||
---|---|---|
9 | I don't have a string preference, should we move the paper examples? the whole file? |
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp | ||
---|---|---|
9 | I meant the paper examples. I don't have a strong preference too, so in case it doesn't matter where the test actually is, please ignore this comment. |
clang/lib/Parse/ParseDecl.cpp | ||
---|---|---|
27 | This looks like an unrelated change after removing the change below. |
I don't understand our consteval implementation enough to approve this, but I didn't see anything to comment on while going through this.
clang/include/clang/Sema/Sema.h | ||
---|---|---|
1071–1075 | I'm annoyed that we do ExprEvalContexts.back() as often as we do, and don't just have Sema::curExprEvalContext. There isn't really anything for you to do here, but I wanted to tell someone of this annoyance, as this is the 3rd time its popped up in a week for me :/ |
Nothing more from me, but I would wait for someone else's approval.
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
18204 | This one seems to be missed. |
Generally looks good, but I did have some questions.
clang/lib/AST/ExprConstant.cpp | ||
---|---|---|
2166–2173 | We can be fancy and reduce a level of indentation, NFC. | |
clang/lib/AST/TextNodeDumper.cpp | ||
286–287 | We should probably also update JSONNodeDumper.cpp similarly so they're somewhat consistent. | |
clang/lib/CodeGen/CodeGenModule.cpp | ||
4215 | ||
6311–6313 | Might as well get fancy here too. | |
clang/lib/Sema/SemaDeclCXX.cpp | ||
2498–2503 | Are these for performance reasons, so we don't traverse into them further? If so, a comment would be helpful explaining that. | |
clang/lib/Sema/SemaExpr.cpp | ||
17958–17965 | Should we be asserting the given expression is either a CallExpr (with a valid callee) or a DeclRefExpr? Otherwise, if called with something else, we'll claim the function found an immediately escalating expression but we won't know *what* caused that. | |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
5013 | I think I'm a bit surprised to see this assert in a function named CheckIf -- I would assume that we'd return false in this case? | |
clang/lib/Serialization/ASTWriterStmt.cpp | ||
626 | Can you explain why this checks that the expression is not immediate escalating? (What test case exercises this?) | |
clang/test/CodeGenCXX/cxx20-consteval-crash.cpp | ||
20 | Why is the cast to void added? | |
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp | ||
9 | Because it's voted in as a DR, we should have a test in clang/test/CXX/drs/ with the appropriate DR number markings (and then regenerate the DR status page as well). |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
---|---|---|
5013 | I think we could but checking whether a non-escalating function is immediate does not make sense to me | |
clang/lib/Serialization/ASTWriterStmt.cpp | ||
626 | Not entirely. My understanding is that we can only use an abbreviation when it's just a reference to a Decl with no additional non-default property, otherwise it needs to be fully serialized (otherwise the "immediate escalating" bit would be lost and deserIAlizing incorrect | |
clang/test/CodeGenCXX/cxx20-consteval-crash.cpp | ||
20 | oups, i wasn't supposed to commit that! |
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp | ||
---|---|---|
9 | I'm not sure core makes dr numbers for papers |
clang/lib/Sema/SemaTemplateDeduction.cpp | ||
---|---|---|
5013 | Okay, that seems reasonable enough. | |
clang/lib/Serialization/ASTWriterStmt.cpp | ||
626 | Ahhhh, okay, that makes sense to me. I was thrown off because it seemed like this was related to ODR captures in lambdas somehow, but I see what you mean now that I look at how other abbreviations are handled. Thanks! | |
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp | ||
9 | Yeah, I think you're right... I can't find a DR number for this either. @Endill -- something to be aware of for DR conformance testing, I guess. |
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp | ||
---|---|---|
9 | Thank you for letting me know! |
clang/lib/Sema/SemaExpr.cpp | ||
---|---|---|
17958–17965 | We should. Doing that, I realized the case of CXXConstructExpr was not handled so now both DeclRef and CXXConstructExpr track whether they are immediate escalating (which is only useful for diagnostics but it's a pretty useful diagnostic) |
Some documentation on the new API would be useful; seems like most calls to isConsteval() should be using isImmediateFunction() instead?