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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
