[PATCH] D151094: [clang] Implement P2564 "consteval must propagate up"

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a subscriber: Endill.
aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5022
+                                                    SourceLocation Loc) {
+  assert(FD->isImmediateEscalating());
+
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > 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?
> I think we could but checking whether a non-escalating function is immediate does not make sense to me
Okay, that seems reasonable enough.


================
Comment at: clang/lib/Serialization/ASTWriterStmt.cpp:629
+      !E->refersToEnclosingVariableOrCapture() && !E->isNonOdrUse() &&
+      !E->isImmediateEscalating()) {
     AbbrevToUse = Writer.getDeclRefExprAbbrev();
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Can you explain why this checks that the expression is not immediate escalating? (What test case exercises this?)
> 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 
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!


================
Comment at: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp:8
+
+namespace examples {
+
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Fznamznon wrote:
> > > cor3ntin wrote:
> > > > Fznamznon wrote:
> > > > > These examples exactly match the ones provided by P2564R3, should they be in a separate test in `CXX` directory then?
> > > > I don't have a string preference, should we move the paper examples? the whole file?
> > > 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.
> > 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'm not sure core makes dr numbers for papers
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D151094/new/

https://reviews.llvm.org/D151094