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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman 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())
----------------
cor3ntin wrote:
> 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
Oh wow, good to ignore that suggestion then! :-D


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18441
       SemaRef.Diag(ND->getLocation(), diag::note_declared_at);
+      if (auto Context =
+              SemaRef.InnermostDeclarationWithDelayedImmediateInvocations()) {
----------------
Fznamznon wrote:
> cor3ntin wrote:
> > 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!
> Uh, yeah, doesn't seem to be improving readability. NVM :)
Heh, I went to make the same suggestion on my last round of review, then checked to see what the actual type would be, and went "eh... probably better the way it is". I'm glad I'm not the only one! :-D


================
Comment at: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp:222
+S2 s = {};
+constinit S2 s2 = {};
+
----------------
Ah, I see, because `s2` is constant initialized, we *don't* hit the call to `side_effect()` and so this is a valid initialization. I think my brain read the code in `f2()` wrong previously, sorry for that!


================
Comment at: clang/test/SemaCXX/cxx2b-consteval-propagate.cpp:245
+  int x = f(0); // expected-note {{undefined function 'f' cannot be used in a constant expression}} \
+                // expected-note {{'SS' is an immediate constructor because the default initializer of 'x' contains a call to a consteval function 'f' and that call is not a constant expression}}
+  SS() = default;
----------------
Thank you for the new diagnostic wording, I think it's a good improvement! However, should the second note be on the constructor declaration as opposed to the declaration of `x`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155175