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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin marked 2 inline comments as done.
cor3ntin added inline comments.


================
Comment at: clang/include/clang/AST/Decl.h:2391
+
+  bool isImmediateFunction() const;
+
----------------
tbaeder wrote:
> Some documentation on the new API would be useful; seems like most calls to `isConsteval()` should be using `isImmediateFunction()` instead?
Yes - I added a comment


================
Comment at: clang/lib/AST/Decl.cpp:3180
+  // consteval specifier,
+  if (isDefaulted() && !isConsteval())
+    return true;
----------------
aaron.ballman wrote:
> Should this be looking at `isExplicitlyDefaulted()` instead?
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 


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2495
     } else {
+      EnterExpressionEvaluationContext Ctx(
+          Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
----------------
Fznamznon wrote:
> What is the point for an additional expression evaluation context here?
Unnecessary, good catch! I think i had to add that in an earlier iteration of the patch which checked for immediate escalation in MarkDeclRefReferenced.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:17966
+
+  getCurFunction()->FoundImmediateEscalatingExpression = true;
+}
----------------
Fznamznon wrote:
> Same is actually for `getCurFunction()`. Can be a `nullptr` if there is no function.
There is an assert above , this function should only be called from a function scope


================
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
----------------
Fznamznon wrote:
> Should the new behavior only be enabled for c++23?
Nah, it was voted as a DR


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151094