[PATCH] D122249: [Clang] Add a compatibiliy warning for non-literals in constexpr.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast accepted this revision.
hubert.reinterpretcast added a comment.
This revision is now accepted and ready to land.

This LGTM (with minor comment). Please wait for Aaron to respond re: the handling of template instantiations.



================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:1912-1913
+                       diag::err_constexpr_local_var_non_literal_type,
+                       isa<CXXConstructorDecl>(Dcl)))
           return false;
         if (!VD->getType()->isDependentType() &&
----------------
Minor nit: The coding standards were updated (some time ago now) to recommend keeping if-else chains consistently braced or not-braced.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122249