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

[Martin Böhme via Phabricator via cfe-commits]

mboehme added a comment.

In D126061#3590896 <https://reviews.llvm.org/D126061#3590896>, @dyung wrote:

> @mboehme, one of our internal tests started to fail to compile after this change due to the compiler no longer accepting what I think should be a valid attribute declaration. I have filed issue #56077 for the problem, can you take a look?

Attributes are not allowed to occur in this position -- I'll discuss in more detail on #56077.

See also the response I'm making on a code comment, where a reviewer pointed out to me that the C++ standard does not allow `[[]]` attributes before `extern "C"`.



================
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:
> 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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126061