This code doesn't seem to match the comment (it matches the code below though).
Ouch, this is pretty expensive because it actually evaluates the constant expression (and promptly forgets what it evaluated).
|2495 ↗||(On Diff #524491)|
What is the point for an additional expression evaluation context here?
CallExpr::getCallee() can return nullptr for example for indirect calls. Probably makes sense to check that it is not nullptr before accessing it.
Same is actually for getCurFunction(). Can be a nullptr if there is no function.
Should the new behavior only be enabled for c++23?
New line is not needed here, I guess.
These examples exactly match the ones provided by P2564R3, should they be in a separate test in CXX directory then?
Yes - I added a comment
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
|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.
There is an assert above , this function should only be called from a function scope
Nah, it was voted as a DR
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.
I don't understand our consteval implementation enough to approve this, but I didn't see anything to comment on while going through this.
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 :/
Generally looks good, but I did have some questions.
We can be fancy and reduce a level of indentation, NFC.
We should probably also update JSONNodeDumper.cpp similarly so they're somewhat consistent.
Might as well get fancy here too.
Are these for performance reasons, so we don't traverse into them further? If so, a comment would be helpful explaining that.
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.
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?
Can you explain why this checks that the expression is not immediate escalating? (What test case exercises this?)
Why is the cast to void added?
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).
I think we could but checking whether a non-escalating function is immediate does not make sense to me
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
oups, i wasn't supposed to commit that!
Okay, that seems reasonable enough.
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!
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.
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)