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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
