[PATCH] D155175: [Clang] Fix consteval propagation for aggregates and defaulted constructors

[Corentin Jabot via Phabricator via cfe-commits]

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


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2441
 bool Sema::CheckImmediateEscalatingFunctionDefinition(
-    FunctionDecl *FD, bool HasImmediateEscalatingExpression) {
-  if (!FD->hasBody() || !getLangOpts().CPlusPlus20 ||
-      !FD->isImmediateEscalating())
+    FunctionDecl *FD, const sema::FunctionScopeInfo *FSI) {
+  if (!getLangOpts().CPlusPlus20 || !FD->isImmediateEscalating())
----------------
aaron.ballman wrote:
> Fznamznon wrote:
> > Is there any logical value in passing `FunctionScopeInfo` instead of a bool flag?
> 
FYI FunctionScopeInfo is declared in `sema` and removing that does not seem possible.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2441
 bool Sema::CheckImmediateEscalatingFunctionDefinition(
-    FunctionDecl *FD, bool HasImmediateEscalatingExpression) {
-  if (!FD->hasBody() || !getLangOpts().CPlusPlus20 ||
-      !FD->isImmediateEscalating())
+    FunctionDecl *FD, const sema::FunctionScopeInfo *FSI) {
+  if (!getLangOpts().CPlusPlus20 || !FD->isImmediateEscalating())
----------------
cor3ntin wrote:
> aaron.ballman wrote:
> > Fznamznon wrote:
> > > Is there any logical value in passing `FunctionScopeInfo` instead of a bool flag?
> > 
> FYI FunctionScopeInfo is declared in `sema` and removing that does not seem possible.
in `~SynthesizedFunctionScope()` we do not have a definition for it. the alternative would be to move SynthesizedFunctionScope to Sema.cpp


================
Comment at: clang/lib/Sema/SemaExpr.cpp:6221
 
+  CXXThisScopeRAII This(*this, Field->getParent(), Qualifiers(),
+                        Field->getParent() != nullptr);
----------------
Fznamznon wrote:
> Could you please describe why this change is required?
Sure, if we have an immediate invocation that refers to the `this` pointer it might need to be transformed, and that transformation will call `getCurrentThisType` which needs `this` to point to the right type


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18441
       SemaRef.Diag(ND->getLocation(), diag::note_declared_at);
+      if (auto Context =
+              SemaRef.InnermostDeclarationWithDelayedImmediateInvocations()) {
----------------
Fznamznon wrote:
> I would prefer spelling type here.
that's an `std::optional<ExpressionEvaluationContextRecord::InitializationContext>`, I'm not sure spelling it improved readability. But i can change it!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155175