Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[clang] Implement P2564 "consteval must propagate up"
ClosedPublic

Authored by cor3ntin on May 22 2023, 6:52 AM.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
aaron.ballman added inline comments.May 22 2023, 7:12 AM
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
18014

Ouch, this is pretty expensive because it actually evaluates the constant expression (and promptly forgets what it evaluated).

clang/lib/Sema/SemaTemplateDeduction.cpp
5037–5039
tbaeder added inline comments.
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?

cor3ntin updated this revision to Diff 524408.May 22 2023, 11:36 AM
cor3ntin marked 9 inline comments as done.
  • Address Aaron's comments
  • Track which expressions are immediately escalating to offer better diagnostics
cor3ntin retitled this revision from [clang][wip] Implement P2564 "consteval must propagate up" to [clang] Implement P2564 "consteval must propagate up".May 22 2023, 11:37 AM
cor3ntin updated this revision to Diff 524491.May 22 2023, 2:26 PM

In FunctionScopeInfo, only track that an immediate-escalating expression
has been found rather than which one - as that is recorded on
the expression itself.

Fznamznon added inline comments.May 23 2023, 4:06 AM
clang/lib/Parse/ParseDecl.cpp
2495 ↗(On Diff #524491)

What is the point for an additional expression evaluation context here?

clang/lib/Sema/SemaDeclCXX.cpp
2477
clang/lib/Sema/SemaExpr.cpp
17982

CallExpr::getCallee() can return nullptr for example for indirect calls. Probably makes sense to check that it is not nullptr before accessing it.

17988

Same is actually for getCurFunction(). Can be a nullptr if there is no function.

18226–18229
18228
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?

cor3ntin updated this revision to Diff 524656.May 23 2023, 4:51 AM
cor3ntin marked 7 inline comments as done.

Address some of Mariya's comments

cor3ntin updated this revision to Diff 524660.May 23 2023, 4:59 AM

Remove unecessary evaluation context.

cor3ntin marked 2 inline comments as done.May 23 2023, 5:07 AM
cor3ntin added inline comments.
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 ↗(On Diff #524491)

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
17988

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?

cor3ntin updated this revision to Diff 524674.May 23 2023, 5:58 AM
cor3ntin marked 2 inline comments as done.

Improve diagnostic message for immediate calls and add Mariya's examples

Out of curiosity - does something like these examples - https://godbolt.org/z/Eqb58Wqoo work as expected?

Yes! I added those as tests, thanks!

cor3ntin marked an inline comment as done.May 23 2023, 6:05 AM
cor3ntin added inline comments.
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
9

I don't have a string preference, should we move the paper examples? the whole file?

Fznamznon added inline comments.May 23 2023, 10:00 AM
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.

shafik added a reviewer: Restricted Project.May 23 2023, 12:45 PM
shafik added inline comments.May 23 2023, 1:35 PM
clang/lib/Parse/ParseDecl.cpp
27 ↗(On Diff #524674)

This looks like an unrelated change after removing the change below.

cor3ntin updated this revision to Diff 525054.May 24 2023, 1:15 AM

Revert unrelated change

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
18228

This one seems to be missed.

cor3ntin updated this revision to Diff 528205.Jun 4 2023, 4:33 AM

Remove missed and useless llvm:: specifier

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
4234
6335–6337

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
17980–17987

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
5022

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
629

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).

cor3ntin added inline comments.Jun 6 2023, 9:23 AM
clang/lib/Sema/SemaTemplateDeduction.cpp
5022

I think we could but checking whether a non-escalating function is immediate does not make sense to me

clang/lib/Serialization/ASTWriterStmt.cpp
629

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!

cor3ntin marked 9 inline comments as done.Jun 6 2023, 9:41 AM
cor3ntin added inline comments.
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
9

I'm not sure core makes dr numbers for papers

aaron.ballman added inline comments.
clang/lib/Sema/SemaTemplateDeduction.cpp
5022

Okay, that seems reasonable enough.

clang/lib/Serialization/ASTWriterStmt.cpp
629

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.

Endill added inline comments.Jun 6 2023, 9:49 AM
clang/test/SemaCXX/cxx2b-consteval-propagate.cpp
9

Thank you for letting me know!

cor3ntin updated this revision to Diff 528919.Jun 6 2023, 10:06 AM

Address Aaron's feedback

cor3ntin updated this revision to Diff 528961.Jun 6 2023, 11:34 AM

Track immediate escalating construct expressions

cor3ntin added inline comments.Jun 6 2023, 11:36 AM
clang/lib/Sema/SemaExpr.cpp
17980–17987

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)

cor3ntin marked 6 inline comments as done.Jun 6 2023, 11:37 AM
This revision is now accepted and ready to land.Jun 7 2023, 10:18 AM
This revision was automatically updated to reflect the committed changes.