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

[Mariya Podchishchaeva via Phabricator via cfe-commits]

Fznamznon added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2495
     } else {
+      EnterExpressionEvaluationContext Ctx(
+          Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
----------------
What is the point for an additional expression evaluation context here?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2477
+      if (const auto *DR =
+              llvm::dyn_cast<DeclRefExpr>(E->getCallee()->IgnoreImplicit());
+          DR && DR->isImmediateEscalating()) {
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:17960
+    if (auto *DeclRef =
+            dyn_cast<DeclRefExpr>(Call->getCallee()->IgnoreImplicit()))
+      DeclRef->setIsImmediateEscalating(true);
----------------
`CallExpr::getCallee()` can return `nullptr` for example for indirect calls. Probably makes sense to check that it is not `nullptr` before accessing it.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17966
+
+  getCurFunction()->FoundImmediateEscalatingExpression = true;
+}
----------------
Same is actually for `getCurFunction()`. Can be a `nullptr` if there is no function.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18202-18203
   for (auto *DR : Rec.ReferenceToConsteval) {
-    NamedDecl *ND = cast<FunctionDecl>(DR->getDecl());
-    if (auto *MD = llvm::dyn_cast<CXXMethodDecl>(ND);
+    const auto *FD = cast<FunctionDecl>(DR->getDecl());
+    const NamedDecl *ND = cast<FunctionDecl>(DR->getDecl());
+    if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(ND);
----------------



================
Comment at: clang/lib/Sema/SemaExpr.cpp:18204
+    const NamedDecl *ND = cast<FunctionDecl>(DR->getDecl());
+    if (const auto *MD = llvm::dyn_cast<CXXMethodDecl>(ND);
         MD && (MD->isLambdaStaticInvoker() || isLambdaCallOperator(MD)))
----------------



================
Comment at: clang/test/SemaCXX/cxx2a-consteval-default-params.cpp:1
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++20 %s
 // RUN: %clang_cc1 -fsyntax-only -verify -std=c++23 %s
----------------
Should the new behavior only be enabled for c++23?


================
Comment at: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp:1
+
+// RUN: %clang_cc1 -std=c++2a -emit-llvm-only -Wno-unused-value %s -verify
----------------
New line is not needed here, I guess.


================
Comment at: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp:8
+
+namespace examples {
+
----------------
These examples exactly match the ones provided by P2564R3, should they be in a separate test in `CXX` directory then?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151094