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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Generally looks good, but I did have some questions.



================
Comment at: clang/lib/AST/ExprConstant.cpp:2166-2173
   if (auto *FD = dyn_cast_or_null<FunctionDecl>(BaseVD)) {
-    if (FD->isConsteval()) {
+    if (FD->isImmediateFunction()) {
       Info.FFDiag(Loc, diag::note_consteval_address_accessible)
           << !Type->isAnyPointerType();
       Info.Note(FD->getLocation(), diag::note_declared_at);
       return false;
     }
----------------
We can be fancy and reduce a level of indentation, NFC.


================
Comment at: clang/lib/AST/TextNodeDumper.cpp:286-287
       OS << " consteval";
+    else if (FD->isImmediateFunction())
+      OS << " immediate";
     if (FD->isMultiVersion())
----------------
We should probably also update JSONNodeDumper.cpp similarly so they're somewhat consistent.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:4234
+  assert(!cast<FunctionDecl>(GD.getDecl())->isImmediateFunction() &&
          "consteval function should never be emitted");
   // If there was no specific requested type, just convert it now.
----------------



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6335-6337
   if (auto *FD = dyn_cast<FunctionDecl>(D))
-    if (FD->isConsteval())
+    if (FD->isImmediateFunction())
       return;
----------------
Might as well get fancy here too.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2498-2503
+    bool TraverseDecl(Decl *) { return true; }
+
+    bool TraverseType(QualType T) { return true; }
+
+    bool VisitBlockExpr(BlockExpr *T) { return true; }
+
----------------
Are these for performance reasons, so we don't traverse into them further? If so, a comment would be helpful explaining that.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17980-17987
+  if (auto *Call = dyn_cast<CallExpr>(E->IgnoreImplicit());
+      Call && Call->getCallee()) {
+    if (auto *DeclRef =
+            dyn_cast<DeclRefExpr>(Call->getCallee()->IgnoreImplicit()))
+      DeclRef->setIsImmediateEscalating(true);
+  } else if (auto *DeclRef = dyn_cast<DeclRefExpr>(E->IgnoreImplicit())) {
+    DeclRef->setIsImmediateEscalating(true);
----------------
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.


================
Comment at: clang/lib/Sema/SemaTemplateDeduction.cpp:5022
+                                                    SourceLocation Loc) {
+  assert(FD->isImmediateEscalating());
+
----------------
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?


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


================
Comment at: clang/test/CodeGenCXX/cxx20-consteval-crash.cpp:20
 consteval X g() { return {0}; }
-void f() { g(); }
+void f() { (void)g(); }
 
----------------
Why is the cast to `void` added?


================
Comment at: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp:8
+
+namespace examples {
+
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151094