aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM
================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1905
+ if (SemaRef.LangOpts.CPlusPlus2b) {
+ if (!VD->getType()->isLiteralType(SemaRef.Context))
+ SemaRef.Diag(VD->getLocation(),
----------------
hubert.reinterpretcast wrote:
> cor3ntin wrote:
> > aaron.ballman wrote:
> > > aaron.ballman wrote:
> > > > cor3ntin wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > This seems to trigger even when the type is dependent:
> > > > > > ```
> > > > > > <stdin>:1:36: warning: definition of a variable of non-literal type
> > > > > > in a constexpr function is incompatible with C++ standards before
> > > > > > C++2b [-Wpre-c++2b-compat]
> > > > > > auto qq = [](auto x) { decltype(x) n; };
> > > > > > ^
> > > > > > 1 warning generated.
> > > > > > ```
> > > > > >
> > > > > > This also seems to emit even when `Kind` is not
> > > > > > `Sema::CheckConstexprKind::Diagnose` (unlike the
> > > > > > `static`/`thread_local` case above). Is the `CheckLiteralType`
> > > > > > logic not reusable for this?
> > > > > You are right, thanks for noticing that, it was rather bogus.
> > > > > The reason I'm not using CheckLiteralType is to avoid duplicating a
> > > > > diagnostics message, as CheckLiteralType doesn't allow us to pass
> > > > > parameter to the diagnostic message.
> > > > >
> > > > > It leaves us with an uncovered scenario though: We do not emit the
> > > > > warning on template instantiation, and I don't think there is an
> > > > > easy way to do that.
> > > > > The reason I'm not using CheckLiteralType is to avoid duplicating a
> > > > > diagnostics message, as CheckLiteralType doesn't allow us to pass
> > > > > parameter to the diagnostic message.
> > > >
> > > > Huh?
> > > >
> > > > ```
> > > > static bool CheckLiteralType(Sema &SemaRef, Sema::CheckConstexprKind
> > > > Kind,
> > > > SourceLocation Loc, QualType T, unsigned
> > > > DiagID,
> > > > Ts &&...DiagArgs) {
> > > > ...
> > > > }
> > > > ```
> > > > I would hope `DiagArgs` should do exactly that? :-)
> > > > It leaves us with an uncovered scenario though: We do not emit the
> > > > warning on template instantiation, and I don't think there is an easy
> > > > way to do that.
> > >
> > > I believe the code paths that lead us here all come from
> > > `Sema::CheckConstexprFunctionDefinition()` which is called from
> > > `Sema::ActOnFinishFunctionBody()` which seems to be called when
> > > instantiating templates in `Sema::InstantiateFunctionDefinition()`, so
> > > perhaps some more investigation is needed as to why we're not reaching
> > > this for template instantiations.
> > We could add something in addition of
> > `Sema::CheckConstexprKind::CheckValid` and
> > `Sema::CheckConstexprKind::Diagnose`, but
> >
> > * not for implicit lambdas, because we should not warn on lambdas that
> > won't be constexpr
> > * for explicit constexpr lambdas / functions, it would force us to call
> > CheckConstexprFunctionDefinition on instanciation, which we currently
> > don't do, and is not free for that one warning - and we would have to
> > not-reemit the other warnings. It seems like quite a fair amount of work
> > for a diagnostic not enabled by default.
> > so perhaps some more investigation is needed as to why we're not reaching
> > this for template instantiations.
>
> @aaron.ballman, do you have any position on whether we want this
> investigation before moving forward with this patch?
>>so perhaps some more investigation is needed as to why we're not reaching
>>this for template instantiations.
> @aaron.ballman, do you have any position on whether we want this
> investigation before moving forward with this patch?
@hubert.reinterpretcast -- I think @cor3ntin did that investigation and found
that we don't make it to this code path because of
https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaDecl.cpp#L14961.
I think this is incremental progress, and we can handle the template
instantiation cases in a follow-up.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D122249/new/
https://reviews.llvm.org/D122249
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
