Page MenuHomePhabricator

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

Authored by faisalv on Dec 1 2017, 2:12 AM.

Details

Reviewers
rsmith
miyuki
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).

Diff Detail

Repository
rC Clang

Event Timeline

miyuki created this revision.Dec 1 2017, 2:12 AM
faisalv added inline comments.Dec 1 2017, 3:41 AM
lib/Parse/ParseTemplate.cpp
692 ↗(On Diff #125087)

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 updated this revision to Diff 125119.Dec 1 2017, 5:57 AM

Use explicit lambda capture list.

miyuki marked an inline comment as done.Dec 1 2017, 5:58 AM
rogfer01 added inline comments.Dec 11 2017, 1:43 AM
lib/Parse/ParseTemplate.cpp
702 ↗(On Diff #125119)

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 updated this revision to Diff 126325.Dec 11 2017, 2:59 AM

Added a test for thead_local.

miyuki marked an inline comment as done.Dec 11 2017, 2:59 AM
faisalv accepted this revision.Dec 17 2017, 1:11 PM

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 ↗(On Diff #126325)

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()

This revision is now accepted and ready to land.Dec 17 2017, 1:11 PM
miyuki updated this revision to Diff 127324.Dec 18 2017, 3:39 AM

Merged two diagnostics into one

miyuki marked an inline comment as done.Dec 18 2017, 3:40 AM

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

faisalv requested changes to this revision.Dec 19 2017, 8:33 PM

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 ...

This revision now requires changes to proceed.Dec 19 2017, 8:33 PM

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).

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

Thanks!

faisalv commandeered this revision.Dec 20 2017, 8:46 PM
faisalv edited reviewers, added: miyuki; removed: faisalv.
faisalv updated this revision to Diff 127828.Dec 20 2017, 8:55 PM
faisalv retitled this revision from [Parser] Diagnose storage classes in template parameter declarations to Diagnose invalid decl-specifiers in non-type template parameter declarations (original author miyuki!).
faisalv edited the summary of this revision. (Show Details)
faisalv set the repository for this revision to rC Clang.
faisalv added projects: Restricted Project, Restricted Project.

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 accepted this revision.Dec 21 2017, 6:07 AM

LGTM, thanks a lot for fixing the patch.

This revision is now accepted and ready to land.Dec 21 2017, 6:07 AM