aaron.ballman added a comment.

In D122249#3402331 <https://reviews.llvm.org/D122249#3402331>, @cor3ntin wrote:

> - Fix test
> - Add a comment explaining why we do not use CheckLiteralType
>
> @aaron,ballman I completely missed that...!

No worries!

> However, CheckLiteralType does a bunch of tests that we do not need
> to diagnose *why* a type is not literal, which we do not care about here.

I think those kinds of diagnostics could be useful to a developer who has 
enabled this compat warning. It may not be immediately clear why the type is 
not a literal, and those notes can help some folks get to that answer much more 
quickly. Do you have some examples of whether the diagnostics clearly are 
distracting or useless?



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1905
+        if (SemaRef.LangOpts.CPlusPlus2b) {
+          if (!VD->getType()->isLiteralType(SemaRef.Context))
+            SemaRef.Diag(VD->getLocation(),
----------------
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.


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

Reply via email to