Diagnose invalid decl-specifiers in non-type template parameter declarations (original author miyuki!)

[faisalv]

Summary

Diagnose invalid decl-specifiers in non-type template parameter declarations

According to the C++ Standard [temp.param]p2:

A storage class shall not be specified in a template-parameter
declaration.

Additionally other decl-specifiers are also restricted.

This patch implements a diagnostic for these restrictions within Sema (inspired by how the restrictions are enforced within ActOnParamDeclarator).

[faisalv]

lib/Parse/ParseTemplate.cpp

692

I tend to prefer explicit captures (unless you have a good reason?) - favoring a clearer description of intention - and acknowledgement of required context - over programming convenience ...

[miyuki]

Use explicit lambda capture list.

[miyuki]

Ping

[rogfer01]

lib/Parse/ParseTemplate.cpp

702

You added tests for SCS_* but not for TSCS_*. Is it worth adding at least one for the latter as well?

template <thread_local int X> struct Z2; // expected-error {{storage class specified for template parameter 'X'}}

[miyuki]

Added a test for thead_local.

[faisalv]

Otherwise, I think this looks good enough to commit.

Do you have commit access? If not, let me know when you're ready for me to commit it on your behalf ...

Thank you for fixing this!

include/clang/Basic/DiagnosticParseKinds.td

671

What about folding both of these diagnostics into one with something along these lines:
def err_storage_class_template_parameter : Error<

"storage class specified for template parameter %select{|%1}0">;

And for no identifier, do << 0, and for the valid identifier case: do << 1 << ParamDecl.getIdentifier()

[miyuki]

Merged two diagnostics into one

[miyuki]

I don't have commit access. Please commit the patch on my behalf.

[faisalv]

Hmm - I think i might make some tweaks to this patch (to be largely symmetric with the similar handling of invalid decl-specifiers on function parameters in Sema::Actions.ActOnParamDeclarator)...

Would you want to try to make those changes and update this diff - or would you rather I make them and commit a patch acknowledging your contribution?

Sorry about not suggesting the changes earlier - but it occurred to me while I was re-reviewing the patch prior to committing it, that we don't handle inline and virtual - which led me to ActOnParamDeclarator ...

[miyuki]

I would appreciate, if you make those changes (because I'm very new to Clang and I'm not sure that I understand how to move these checks to Sema correctly).

[faisalv]

Sounds good - if I don't get this done over the next seven days - would you mind just pinging me!

Thanks!

[faisalv]

Miyuki - please take a look at the patch and let me know if you agree with the changes - or have any concerns...

Thank you!

P.S. While I can see the argument for having these syntactical checks be done by the Parser - since they could be described as context sensitive syntax analysis, moving the changes to Sema also seems quite appropriate.

[miyuki]

LGTM, thanks a lot for fixing the patch.

[faisalv]

Added via https://reviews.llvm.org/rC321339

include/clang/Basic/DiagnosticParseKinds.td

def err_explicit_instantiation_enum : Error<
   "enumerations cannot be explicitly instantiated">;
 def err_expected_template_parameter : Error<"expected template parameter">;
 
 def err_missing_dependent_template_keyword : Error<
   "use 'template' keyword to treat '%0' as a dependent template name">;
 def warn_missing_dependent_template_keyword : ExtWarn<
   "use 'template' keyword to treat '%0' as a dependent template name">;
 
+def err_storage_class_template_parameter : Error<
+  "storage class specified for template parameter%select{| %1}0">;
+
 def ext_extern_template : Extension<
   "extern templates are a C++11 extension">, InGroup<CXX11>;
 def warn_cxx98_compat_extern_template : Warning<
   "extern templates are incompatible with C++98">,
   InGroup<CXX98CompatPedantic>, DefaultIgnore;
 def warn_static_inline_explicit_inst_ignored : Warning<
   "ignoring '%select{static|inline}0' keyword on explicit template "
   "instantiation">, InGroup<DiagGroup<"static-inline-explicit-instantiation">>;

lib/Parse/ParseTemplate.cpp

Parser::ParseNonTypeTemplateParameter(unsigned Depth, unsigned Position) {
   // Parse this as a typename.
   Declarator ParamDecl(DS, Declarator::TemplateParamContext);
   ParseDeclarator(ParamDecl);
   if (DS.getTypeSpecType() == DeclSpec::TST_unspecified) {
     Diag(Tok.getLocation(), diag::err_expected_template_parameter);
     return nullptr;
   }
 
+  // C++ [temp.param]p2:
+  //   A storage class shall not be specified in a template-parameter
+  //   declaration.
+  auto ReportStorageClass = [this, &ParamDecl](SourceLocation Loc) {
+    if (ParamDecl.getIdentifier())
+      Diag(Loc, diag::err_storage_class_template_parameter)
+          << 1 << ParamDecl.getIdentifier() << FixItHint::CreateRemoval(Loc);
+    else
+      Diag(Loc, diag::err_storage_class_template_parameter)
+          << 0 << FixItHint::CreateRemoval(Loc);
+  };
+  if (DS.getStorageClassSpec() != DeclSpec::SCS_unspecified)
+    ReportStorageClass(DS.getStorageClassSpecLoc());
+  if (DS.getThreadStorageClassSpec() != DeclSpec::TSCS_unspecified)
+    ReportStorageClass(DS.getThreadStorageClassSpecLoc());
+
   // Recover from misplaced ellipsis.
   SourceLocation EllipsisLoc;
   if (TryConsumeToken(tok::ellipsis, EllipsisLoc))
     DiagnoseMisplacedEllipsisInDeclarator(EllipsisLoc, ParamDecl);
 
   // If there is a default value, parse it.
   // Per C++0x [basic.scope.pdecl]p9, we parse the default argument before
   // we introduce the template parameter into the local scope.

test/CXX/temp/temp.param/p2-cpp11.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++11 %s
+
+// A storage class shall not be specified in a template-parameter declaration.
+template<thread_local int Value> struct Z0; // expected-error {{storage class specified for template parameter 'Value'}}
+template<thread_local int> struct Z1; // expected-error {{storage class specified for template parameter}}

test/CXX/temp/temp.param/p2.cpp

 // RUN: %clang_cc1 -fsyntax-only -verify %s
-// expected-no-diagnostics
 
 // There is no semantic difference between class and typename in a
 // template-parameter. typename followed by an unqualified-id names a
 // template type parameter.
 template<class T> struct X;
 template<typename T> struct X;
 
 // typename followed by aqualified-id denotes the type in a non-type
 // parameter-declaration.
 template<typename T, typename T::type Value> struct Y0;
 template<typename T, typename X<T>::type Value> struct Y1;
 
 // A storage class shall not be specified in a template-parameter declaration.
-template<static int Value> struct Z; // FIXME: expect an error
+template<static int Value> struct Z0; // expected-error {{storage class specified for template parameter 'Value'}}
+template<extern int> struct Z1; // expected-error {{storage class specified for template parameter}}
 
 // Make sure that we properly disambiguate non-type template parameters that
 // start with 'class'.
 class X1 { };
 template<class X1 *xptr> struct Y2 { };
 
 // FIXME: add the example from p2