[PATCH] D126061: [clang] Reject non-declaration C++11 attributes on declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Parse/Parser.cpp:1164
       DS.getParsedSpecifiers() == DeclSpec::PQ_StorageClassSpecifier) {
-    Decl *TheDecl = ParseLinkage(DS, DeclaratorContext::File);
+    Decl *TheDecl = ParseLinkage(DS, DeclaratorContext::File, Attrs);
     return Actions.ConvertDeclToDeclGroup(TheDecl);
----------------
mboehme wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > rsmith wrote:
> > > > We should `ProhibitAttrs` here rather than passing them on.
> > > > ```
> > > > [[]] extern "C" void f();
> > > > ```
> > > > ... is invalid. (Per the grammar in https://eel.is/c++draft/dcl.dcl#dcl.pre-1 and https://eel.is/c++draft/dcl.dcl#dcl.link-2 an attribute-specifier-seq can't appear here.)
> > > +1, looks like we're missing test coverage for that case (but those diagnostics by GCC or MSVC... hoo boy!): https://godbolt.org/z/cTfPbK837
> > Fixed and added a test in test/Parser/cxx0x-attributes.cpp.
> Update: @dyung is seeing errors on their codebase because of this -- see also the comment they added to this patch.
> 
> It's not unexpected that people would have this incorrect usage in their codebases because we used to allow it. At the same time, it's not hard to fix, and I would generally expect this kind of error to occur in first-party code (that is easily fixed) rather than third-party libraries, because the latter usually compile on GCC, and GCC disallows this usage.
> 
> Still, I wanted to discuss whether you think we need to reinstate support for this (incorrect) usage temporarily, with a deprecation warning rather than an error?
> Still, I wanted to discuss whether you think we need to reinstate support for this (incorrect) usage temporarily, with a deprecation warning rather than an error?

I think it depends heavily on what's been impacted. If it's application code, the error is fine because it's pretty trivial to fix. If it's system header or popular 3rd party library code, then a warning might make more sense.

It's worth noting that what we accepted didn't have any semantic impact anyway. We would accept the attribute, but not actually add it into the AST: https://godbolt.org/z/q7n1794Tx. So I'm not certain there's any harm aside from the annoyance of getting an error where you didn't previously get one before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126061