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

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

mboehme added a comment.

Fix in review at https://reviews.llvm.org/D128499


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

[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#3605276 , @miyuki wrote:

> Hi Martin,
>
> I think this patch caused a regression in diagnostics. Clang used to warn 
> about the following code:

[snip]

> And now it doesn't. Could you please have a look into it?

Thanks for alerting me to this! I'll investigate and will submit a fix.


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126061#3605276 , @miyuki wrote:

> Hi Martin,
>
> I think this patch caused a regression in diagnostics. Clang used to warn 
> about the following code:
>
>   struct X {
>   [[deprecated]] struct Y {};
>   };
>
>
>
>   $ clang -std=c++17 -Wall test.cpp
>   :2:7: warning: attribute 'deprecated' is ignored, place it after 
> "struct" to apply attribute to type declaration [-Wignored-attributes]
>   [[deprecated]] struct Y {};
> ^
>
> And now it doesn't. Could you please have a look into it?

Good catch, that does appear to be a regression:

  http://eel.is/c++draft/class#nt:member-declaration
  http://eel.is/c++draft/class#mem.general-14


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

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

[Mikhail Maltsev via Phabricator via cfe-commits]

miyuki added a comment.

Hi Martin,

I think this patch caused a regression. Clang used to warn about the following 
code:

  struct X {
  [[deprecated]] struct Y {};
  };



  $ clang -std=c++17 -Wall test.cpp
  :2:7: warning: attribute 'deprecated' is ignored, place it after 
"struct" to apply attribute to type declaration [-Wignored-attributes]
  [[deprecated]] struct Y {};
^

And now it doesn't. Could you please have a look into it?


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

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

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

mboehme added a comment.

https://reviews.llvm.org/D128097 is now submitted. Unfortunately, it didn't fix 
the compile-time regression entirely (see detailed comments there).

I'll do some more investigation to see where the rest of the slowdown is coming 
from.


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

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

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

mboehme added a comment.

Fix for compile time regression submitted for review at 
https://reviews.llvm.org/D128097


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

[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#3585681 , @nikic wrote:

> FYI this change had a measurable effect on compile-time 
> (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb=instructions),
>  about 0.5% regression for `O0` builds. Not sure if that's expected.

I've found the reason for this slowdown, and a fix.

In a number of places, this patch adds additional local variables of type 
`ParsedAttributes`, typically because we need to keep track of declaration and 
decl-specifier-seq attributes in two separate `ParsedAttribute` lists where 
previously we were putting them in the same list. Note that in the vast 
majority of cases, these `ParsedAttributes` lists are empty.

I would have assumed that creating an empty `ParsedAttributes`, potentially 
iterating over it, then destroying it, is cheap. However, this is not true for 
`ParsedAttributes` because it uses a `TinyPtrVector` as the underlying 
container. The same is true for the `AttributePool` that `ParsedAttributes` 
contains.

`TinyPtrVector` is amazingly memory-frugal in that it consumes only a single 
pointer worth of memory if the vector contains zero or one elements. However, 
this comes at a cost: `begin()` and `end()` on this container are relatively 
expensive. As a result, iterating over an empty `TinyPtrVector`


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

[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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , @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

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Douglas Yung via Phabricator via cfe-commits]

dyung added a comment.

@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?


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

[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#3585681 , @nikic wrote:

> FYI this change had a measurable effect on compile-time 
> (http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb=instructions),
>  about 0.5% regression for `O0` builds. Not sure if that's expected.

Thanks for the heads-up.

Without more extensive investigation, it's hard to say. I wasn't necessarily 
expecting a slowdown, but I can come up with various hypotheses for what might 
be causing it.

I've seen the instructions on the About 
 page on how to reproduce 
results and will give that a go.

The first thing to analyze would be whether the slowdown occurs only on code 
that uses attributes (heavily) or also on code that doesn't use attributes.

There isn't any single substantial change in the code that would cause an 
obvious slowdown. I could speculate on some potential causes, but it seems 
better to measure instead. Is there any particular tooling that you would 
recommend for comparing profiler runs from two different compiler versions (one 
with my patch, one without)?

I'll post an update here when I find out more.


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

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

[Nikita Popov via Phabricator via cfe-commits]

nikic added a comment.

FYI this change had a measurable effect on compile-time 
(http://llvm-compile-time-tracker.com/compare.php?from=7acc88be0312c721bc082ed9934e381d297f4707=8c7b64b5ae2a09027c38db969a04fc9ddd0cd6bb=instructions),
 about 0.5% regression for `O0` builds. Not sure if that's expected.


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

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

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

mboehme marked 2 inline comments as done.
mboehme added a comment.

Thank you @aaron.ballman and @rsmith for the careful and productive reviews!




Comment at: clang/lib/Parse/ParseStmt.cpp:229
   default: {
+bool HaveAttrs = !(CXX11Attrs.empty() && GNUAttrs.empty());
+auto IsStmtAttr = [](ParsedAttr ) { return Attr.isStmtAttr(); };

aaron.ballman wrote:
> I don't insist, but I think it's a tiny bit easier to read this way.
Done!



Comment at: clang/lib/Parse/ParseStmt.cpp:321-323
+for (const ParsedAttr  : CXX11Attrs) {
+  Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+}

aaron.ballman wrote:
> 
Done!


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

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

LGTM!




Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:153
 // FIXME: Use a better mechanism to determine this.
+// We use this in `isCXX11Attribute` below, so it _should_ only return
+// true for the `alignas` spelling, but it currently also returns true

mboehme wrote:
> aaron.ballman wrote:
> > Now that C23 has attributes, it's also not clear whether `_Alignas` will be 
> > handled slightly differently in the future as an attribute rather than a 
> > declaration specifier as happened with `_Noreturn` for C23. So agreed this 
> > is a tricky area.
> Thanks for pointing this out! Is there anything specific I should add to the 
> code comment, or is your comment just for general awareness?
Nope, just spreading the information around to more people's brains -- nothing 
needs to be changed here.



Comment at: clang/include/clang/Sema/ParsedAttr.h:1126
+/// `Result`. Sets `Result.Range` to the combined range of `First` and 
`Second`.
+void ConcatenateAttributes(ParsedAttributes , ParsedAttributes ,
+   ParsedAttributes );

mboehme wrote:
> aaron.ballman wrote:
> > I think `Concatenate` implies that `First` and `Second` are untouched, but 
> > that's not really the case here. Perhaps `takeAndConcatenateAll()` or 
> > something along those lines instead? Also, the function should start with a 
> > lowercase letter per the usual coding style rules.
> Good point. Done, with a slightly different name -- WDYT?
Works great for me!



Comment at: clang/lib/Parse/ParseStmt.cpp:229
   default: {
+bool HaveAttrs = !(CXX11Attrs.empty() && GNUAttrs.empty());
+auto IsStmtAttr = [](ParsedAttr ) { return Attr.isStmtAttr(); };

I don't insist, but I think it's a tiny bit easier to read this way.



Comment at: clang/lib/Parse/ParseStmt.cpp:321-323
+for (const ParsedAttr  : CXX11Attrs) {
+  Diag(AL.getRange().getBegin(), diag::warn_attribute_ignored) << AL;
+}





Comment at: clang/lib/Parse/ParseStmt.cpp:230-237
 if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
  (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
  ParsedStmtContext()) &&
 ((GNUAttributeLoc.isValid() &&
-  !(!Attrs.empty() &&
-llvm::all_of(
-Attrs, [](ParsedAttr ) { return Attr.isStmtAttr(); }))) ||
+  !(!(CXX11Attrs.empty() && GNUAttrs.empty()) &&
+llvm::all_of(CXX11Attrs, isStmtAttr) &&
+llvm::all_of(GNUAttrs, isStmtAttr))) ||

mboehme wrote:
> aaron.ballman wrote:
> > The logic is correct here, but this predicate has gotten really difficult 
> > to understand. If you wanted to split some of those conditions out into 
> > named variables for clarity, I would not be sad (but I also don't insist 
> > either).
> Yes, this is confusing. I've factored out some variables that I hope make 
> this more readable.
> 
> By the way, it might look as if `GNUAttributeLoc.isValid()` implies 
> `HaveAttributes` and that the latter is therefore redundant, but this isn’t 
> actually true if we failed to parse the GNU attribute. Removing the 
> `HaveAttributes` makes some tests fail, e.g. line 78 of 
> clang/test/Parser/stmt-attributes.c.
Thanks, I think this is more readable this way!



Comment at: clang/lib/Parse/ParseStmt.cpp:248-251
+  if (CXX11Attrs.Range.getBegin().isValid())
+DeclStart = CXX11Attrs.Range.getBegin();
+  else if (GNUAttrs.Range.getBegin().isValid())
+DeclStart = GNUAttrs.Range.getBegin();

mboehme wrote:
> aaron.ballman wrote:
> > Do CXX attributes always precede GNU ones, or do we need to do a source 
> > location comparison here to see which location is lexically earlier?
> Yes, we can rely on this being the case. This function is called from only 
> one place where both CXX11Attrs and GNUAttrs can potentially contain 
> attributes, namely `ParseStatementOrDeclaration()` (line 114 in this file). 
> There, the CXX11Attrs are parsed before the GNUAttrs. I’ve added an 
> assertion, however, since this guarantee is important to the correctness of 
> the code here.
Thank you for the confirmation and the added assertion!


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

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

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

mboehme marked 11 inline comments as done.
mboehme added a comment.

In D126061#3581350 , @aaron.ballman 
wrote:

> Only thing left for me are the nits I pointed out in the last review, 
> otherwise I think this is all set.

Thanks for the quick turnaround!

> Giving my LG because I don't think we need another round of review for those 
> nits unless you want it

Just two things I'd appreciate a quick confirmation on:

- My question on `isAlignasAttribute()`
- Feedback on whether you think my refactoring of the complex boolean 
expression makes sense




Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:153
 // FIXME: Use a better mechanism to determine this.
+// We use this in `isCXX11Attribute` below, so it _should_ only return
+// true for the `alignas` spelling, but it currently also returns true

aaron.ballman wrote:
> Now that C23 has attributes, it's also not clear whether `_Alignas` will be 
> handled slightly differently in the future as an attribute rather than a 
> declaration specifier as happened with `_Noreturn` for C23. So agreed this is 
> a tricky area.
Thanks for pointing this out! Is there anything specific I should add to the 
code comment, or is your comment just for general awareness?



Comment at: clang/include/clang/Sema/DeclSpec.h:1916
+  /// declaration, i.e. `x` and `y` in this case.
+  Declarator(const DeclSpec , const ParsedAttributesView ,
+ DeclaratorContext C)

aaron.ballman wrote:
> I know it's pre-existing code, but since you're updating the parameters and 
> adding some great comments, can you rename `ds` to `DS` per coding style 
> guides?
No problem -- done!



Comment at: clang/include/clang/Sema/DeclSpec.h:1928-1930
+assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr ) {
+  return AL.isStandardAttributeSyntax();
+}));

aaron.ballman wrote:
> Can you add an `&& "and this is what the assert is testing for"` message here 
> so when the assertion fails there's more details as to what went sideways?
Good point -- done.



Comment at: clang/include/clang/Sema/ParsedAttr.h:1126
+/// `Result`. Sets `Result.Range` to the combined range of `First` and 
`Second`.
+void ConcatenateAttributes(ParsedAttributes , ParsedAttributes ,
+   ParsedAttributes );

aaron.ballman wrote:
> I think `Concatenate` implies that `First` and `Second` are untouched, but 
> that's not really the case here. Perhaps `takeAndConcatenateAll()` or 
> something along those lines instead? Also, the function should start with a 
> lowercase letter per the usual coding style rules.
Good point. Done, with a slightly different name -- WDYT?



Comment at: clang/lib/Parse/ParseStmt.cpp:229
   default: {
+auto isStmtAttr = [](ParsedAttr ) { return Attr.isStmtAttr(); };
 if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||

aaron.ballman wrote:
> Coding style nit.
Done.



Comment at: clang/lib/Parse/ParseStmt.cpp:230-237
 if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
  (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
  ParsedStmtContext()) &&
 ((GNUAttributeLoc.isValid() &&
-  !(!Attrs.empty() &&
-llvm::all_of(
-Attrs, [](ParsedAttr ) { return Attr.isStmtAttr(); }))) ||
+  !(!(CXX11Attrs.empty() && GNUAttrs.empty()) &&
+llvm::all_of(CXX11Attrs, isStmtAttr) &&
+llvm::all_of(GNUAttrs, isStmtAttr))) ||

aaron.ballman wrote:
> The logic is correct here, but this predicate has gotten really difficult to 
> understand. If you wanted to split some of those conditions out into named 
> variables for clarity, I would not be sad (but I also don't insist either).
Yes, this is confusing. I've factored out some variables that I hope make this 
more readable.

By the way, it might look as if `GNUAttributeLoc.isValid()` implies 
`HaveAttributes` and that the latter is therefore redundant, but this isn’t 
actually true if we failed to parse the GNU attribute. Removing the 
`HaveAttributes` makes some tests fail, e.g. line 78 of 
clang/test/Parser/stmt-attributes.c.



Comment at: clang/lib/Parse/ParseStmt.cpp:248-251
+  if (CXX11Attrs.Range.getBegin().isValid())
+DeclStart = CXX11Attrs.Range.getBegin();
+  else if (GNUAttrs.Range.getBegin().isValid())
+DeclStart = GNUAttrs.Range.getBegin();

aaron.ballman wrote:
> Do CXX attributes always precede GNU ones, or do we need to do a source 
> location comparison here to see which location is lexically earlier?
Yes, we can rely on this being the case. This function is called from only one 
place where both CXX11Attrs and GNUAttrs can potentially contain attributes, 
namely 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.

Only thing left for me are the nits I pointed out in the last review, otherwise 
I think this is all set. Giving my LG because I don't think we need another 
round of review for those nits unless you want it




Comment at: clang/lib/Sema/ParsedAttr.cpp:241
+  case AT_ObjCGC:
+  case AT_VectorSize:
+return true;

rsmith wrote:
> mboehme wrote:
> > rsmith wrote:
> > > This one is weird. Given:
> > > 
> > > ```
> > > [[gnu::vector_size(8)]] int x;
> > > int [[gnu::vector_size(8)]] y;
> > > int *[[gnu::vector_size(8)]]* z;
> > > ```
> > > 
> > > - Clang and GCC accept `x`
> > > - Clang rejects `y` and GCC warns that the attribute is ignored on `y`
> > > - Clang accepts `z` with a warning that GCC would ignore the attribute, 
> > > whereas GCC silently accepts
> > > 
> > > I guess after this patch we'll silently accept `x` (treated as the 
> > > "sliding" behavior) and accept `y` and `z` with a warning that it's 
> > > GCC-incompatible?
> > > This one is weird. Given:
> > > 
> > > ```
> > > [[gnu::vector_size(8)]] int x;
> > > int [[gnu::vector_size(8)]] y;
> > > int *[[gnu::vector_size(8)]]* z;
> > > ```
> > > 
> > > - Clang and GCC accept `x`
> > > - Clang rejects `y` and GCC warns that the attribute is ignored on `y`
> > > - Clang accepts `z` with a warning that GCC would ignore the attribute, 
> > > whereas GCC silently accepts
> > 
> > Actually, for z, Clang gives me not just a warning but also an error: 
> > 
> > ```
> > :1:8: warning: GCC does not allow the 'vector_size' attribute to be 
> > written on a type [-Wgcc-compat]
> > int *[[gnu::vector_size(8)]]* z;
> >^
> > :1:8: error: invalid vector element type 'int *'
> > ```
> > 
> > https://godbolt.org/z/E4ss8rzWE
> > 
> > > I guess after this patch we'll silently accept `x` (treated as the 
> > > "sliding" behavior) and accept `y` and `z` with a warning that it's 
> > > GCC-incompatible?
> > 
> > Thanks for pointing this out to me!
> > 
> > Actually, I must not have checked the GCC behavior previously, so I went 
> > too far and gave `vector_size` meaning when placed on `y` instead of just 
> > ignoring it. I’ve now changed the code to ignore the attribute and emit a 
> > warning, like GCC does. (See tests in `vector-gcc-compat.c`.)
> > 
> > It’s particularly surprising to me that GCC allows `vector_size` to be 
> > applied to a pointer type (the `z` case) – particularly so since the 
> > [documentation](https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html) 
> > explicitly states: “The vector_size attribute is only applicable to 
> > integral and floating scalars, although arrays, pointers, and function 
> > return values are allowed in conjunction with this construct.”
> > 
> > I wanted to test whether GCC just silently ignores the attribute when 
> > placed on a pointer or whether it has an effect. Here are some test cases:
> > 
> > https://godbolt.org/z/Ke1E7TnYe
> > 
> > From looking at the generated code, it appears that `vector_size`, when 
> > applied to a pointer type, _does_ have an effect – though a maybe slightly 
> > surprising one: It is applied to the base type, rather than the pointer 
> > type. I suspect that GCC simply treats the C++11 spelling the same as the 
> > `__attribute__` spelling (i.e. it doesn’t apply the stricter C++11 rules on 
> > what the attribute should appertain to), and it seems that both spellings 
> > get applied to the base type of the declaration no matter where they appear 
> > in the declaration (except if the attribute is ignored entirely).
> > 
> > Interestingly, this means that Clang’s warning (“GCC does not allow the 
> > 'vector_size' attribute to be written on a type”) is wrong. Maybe this is 
> > behavior in GCC that has changed since the warning was introduced?
> > 
> > Given all of this, I propose we treat the various cases as follows (and 
> > I’ve implemented this in the code):
> > 
> > * We continue to accept `x` without a warning, just as we do today (same 
> > behavior as GCC)
> > * We allow `y` but warn that the attribute doesn’t have an effect (same 
> > behavior as GCC)
> > * We continue to reject `z`, even though GCC allows it and it has an effect 
> > there. Rationale: a) We reject this today, so this isn’t a regression; b) 
> > this usage is unusual and likely not to occur often in the wild; c) fixing 
> > this to be consistent with GCC will take a non-trivial amount of code, so 
> > I’d like to keep it outside the scope of this change.
> > https://godbolt.org/z/Ke1E7TnYe
> 
> That GCC behavior is shocking. Shocking enough that I think the following 
> approach would make sense:
> 
> 1) For compatibility, emulate the GCC behavior as much as possible for 
> `[[gnu::vector_size]]`, except:
> - Continue to error rather than only warning in the cases where the 
> attribute does not create a vector type at all, and
> - Warn on cases like `int *[[gnu::vector_size(N)]]` 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

This is a bold direction but I like it a lot. Over to @aaron.ballman for final 
approval.




Comment at: clang/lib/Sema/ParsedAttr.cpp:228
+  switch (getParsedKind()) {
+  case AT_AddressSpace:
+  case AT_OpenCLPrivateAddressSpace:

mboehme wrote:
> rsmith wrote:
> > I don't think this one really slides today, in the syntactic sense, prior 
> > to this patch (and I guess the same is true for most of these). Given:
> > 
> > ```
> > [[clang::address_space(5)]] int x;
> > int [[clang::address_space(5)]] y;
> > ```
> > 
> > ... we accept `x` and reject `y`, making it look like this is simply a 
> > declaration attribute. Hm, but we accept:
> > 
> > ```
> > int *[[clang::address_space(5)]] *z;
> > ```
> > 
> > ... so it really *does* seem to behave like a type attribute sometimes?
> > 
> > OK, so this patch is doing two things:
> > 
> > 1) It's permitting the use of these attributes as type attributes, where 
> > they were not consistently permitted previously.
> > 2) It's deprecating / warning on uses of them as declaration attributes.
> > 
> > That means there's no way to write code that is compatible with both old 
> > and new Clang and produces no warnings, but it's easy enough to turn the 
> > warning flag off I suppose. The new behavior does seem much better than the 
> > old.
> > I don't think this one really slides today, in the syntactic sense, prior 
> > to this patch (and I guess the same is true for most of these). Given:
> > 
> > ```
> > [[clang::address_space(5)]] int x;
> > int [[clang::address_space(5)]] y;
> > ```
> > 
> > ... we accept `x` and reject `y`, making it look like this is simply a 
> > declaration attribute.
> 
> Yes, and we get the same behavior for all C++11 attributes.
> 
> This is because, today, Clang flat-out rejects _all_ C++11 attributes that 
> are placed after a decl-specifier-seq:
> 
> From `ParseDeclarationSpecifiers()`:
> 
> ```
> // Reject C++11 attributes that appertain to decl specifiers as
> // we don't support any C++11 attributes that appertain to decl
> // specifiers. This also conforms to what g++ 4.8 is doing.
> ProhibitCXX11Attributes(attrs, diag::err_attribute_not_type_attr);
> ```
> 
> https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L3182
> 
> So really what the "slides" terminology means is "is treated as appertaining 
> to the decl-specifier-seq instead of the declaration".
> 
> > Hm, but we accept:
> > 
> > ```
> > int *[[clang::address_space(5)]] *z;
> > ```
> > 
> > ... so it really *does* seem to behave like a type attribute sometimes?
> 
> Yes, because Clang does allow C++11 attributes (including `address_space` in 
> particular) on declarator chunks today -- it just doesn't allow them on the 
> decl-specifier-seq. As an aside, this makes the error message we get when 
> putting the attribute on the decl-specifier-seq today ("'address_space' 
> attribute cannot be applied to types") pretty misleading.
> 
> > OK, so this patch is doing two things:
> > 
> > 1) It's permitting the use of these attributes as type attributes, where 
> > they were not consistently permitted previously.
> > 2) It's deprecating / warning on uses of them as declaration attributes.
> 
> Actually, 1) happens in https://reviews.llvm.org/D111548, which this change 
> is based on.
> 
> I did things this way because
> 
>   - I want to introduce `annotate_type` first, so that this change can use it 
> in tests as an example of a type attribute that doesn't "slide"
>   - I wanted tests in https://reviews.llvm.org/D111548 to be able to use 
> `annotate_type` in all places that it's intended for, including on the 
> decl-specifier-seq, so I needed to open up that usage. (Actually, when I 
> first authored https://reviews.llvm.org/D111548, I didn't even realize that 
> this followup change would be coming.)
> 
> Is this confusing? I could instead do the following:
> 
>   - Move the change that allows C++11 attributes on the decl-specifier-seq 
> from https://reviews.llvm.org/D111548 to here.
>   - Move tests that use `annotate_type` on the decl-specifier-seq from 
> https://reviews.llvm.org/D111548 to here.
> 
> Do you think that would be preferable?
> 
> > That means there's no way to write code that is compatible with both old 
> > and new Clang and produces no warnings, but it's easy enough to turn the 
> > warning flag off I suppose.
> 
> That's true -- and I think it's the best we can do. The behavior that Clang 
> exhibits today is of course set in stone (error out if the attribute is 
> placed on the decl-specifier-seq), and at the same time it seems obvious that 
> we want to nudge people away from putting the attribute on the declaration, 
> as it's really a type attribute. (Just repeating your analysis here 
> essentially; I think we agree, and there's nothing to do here.)
> 
> I 

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D126061#3577054 , @mboehme wrote:

> @aaron.ballman Any further comments from you on this change?

Mostly just nits from me at this point, plus some answers to questions I had 
missed previously.

> There is some breakage in the pre-merge checks, but it looks as if it's 
> pre-existing breakage. See the comment-only change here that looks as if it 
> exhibits similar breakage:
>
> https://reviews.llvm.org/D127494

I agree that the precommit CI failures look unrelated to this patch.




Comment at: clang/include/clang/Basic/AttributeCommonInfo.h:153
 // FIXME: Use a better mechanism to determine this.
+// We use this in `isCXX11Attribute` below, so it _should_ only return
+// true for the `alignas` spelling, but it currently also returns true

Now that C23 has attributes, it's also not clear whether `_Alignas` will be 
handled slightly differently in the future as an attribute rather than a 
declaration specifier as happened with `_Noreturn` for C23. So agreed this is a 
tricky area.



Comment at: clang/include/clang/Sema/DeclSpec.h:1916
+  /// declaration, i.e. `x` and `y` in this case.
+  Declarator(const DeclSpec , const ParsedAttributesView ,
+ DeclaratorContext C)

I know it's pre-existing code, but since you're updating the parameters and 
adding some great comments, can you rename `ds` to `DS` per coding style guides?



Comment at: clang/include/clang/Sema/DeclSpec.h:1928-1930
+assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr ) {
+  return AL.isStandardAttributeSyntax();
+}));

Can you add an `&& "and this is what the assert is testing for"` message here 
so when the assertion fails there's more details as to what went sideways?



Comment at: clang/include/clang/Sema/ParsedAttr.h:1126
+/// `Result`. Sets `Result.Range` to the combined range of `First` and 
`Second`.
+void ConcatenateAttributes(ParsedAttributes , ParsedAttributes ,
+   ParsedAttributes );

I think `Concatenate` implies that `First` and `Second` are untouched, but 
that's not really the case here. Perhaps `takeAndConcatenateAll()` or something 
along those lines instead? Also, the function should start with a lowercase 
letter per the usual coding style rules.



Comment at: clang/include/clang/Sema/ParsedAttr.h:664
+  /// This may only be called if isStandardAttributeSyntax() returns true.
+  bool slidesFromDeclToDeclSpec() const;
+

mboehme wrote:
> aaron.ballman wrote:
> > Because this is now specific to standard attributes, and those don't 
> > "slide" under normal circumstances, it might be nice to rename this method 
> > to make it more clear that this should not be called normally. I don't have 
> > a strong opinion on what a *good* name is, but even something like 
> > `improperlySlidesFromDeclToDeclSpec()` would make me happy.
> I've decided to add `LegacyBehavior` at the end -- WDYT?
Works for me



Comment at: clang/lib/Parse/ParseStmt.cpp:229
   default: {
+auto isStmtAttr = [](ParsedAttr ) { return Attr.isStmtAttr(); };
 if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||

Coding style nit.



Comment at: clang/lib/Parse/ParseStmt.cpp:230-237
 if ((getLangOpts().CPlusPlus || getLangOpts().MicrosoftExt ||
  (StmtCtx & ParsedStmtContext::AllowDeclarationsInC) !=
  ParsedStmtContext()) &&
 ((GNUAttributeLoc.isValid() &&
-  !(!Attrs.empty() &&
-llvm::all_of(
-Attrs, [](ParsedAttr ) { return Attr.isStmtAttr(); }))) ||
+  !(!(CXX11Attrs.empty() && GNUAttrs.empty()) &&
+llvm::all_of(CXX11Attrs, isStmtAttr) &&
+llvm::all_of(GNUAttrs, isStmtAttr))) ||

The logic is correct here, but this predicate has gotten really difficult to 
understand. If you wanted to split some of those conditions out into named 
variables for clarity, I would not be sad (but I also don't insist either).



Comment at: clang/lib/Parse/ParseStmt.cpp:248-251
+  if (CXX11Attrs.Range.getBegin().isValid())
+DeclStart = CXX11Attrs.Range.getBegin();
+  else if (GNUAttrs.Range.getBegin().isValid())
+DeclStart = GNUAttrs.Range.getBegin();

Do CXX attributes always precede GNU ones, or do we need to do a source 
location comparison here to see which location is lexically earlier?



Comment at: clang/lib/Parse/ParseStmt.cpp:317-318
   case tok::kw_asm: {
-ProhibitAttributes(Attrs);
+ProhibitAttributes(CXX11Attrs);
+ProhibitAttributes(GNUAttrs);
 bool msAsm = false;

mboehme wrote:
> aaron.ballman wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > It seems like we might have 

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

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

mboehme added a comment.

@aaron.ballman Any further comments from you on this change?

There is some breakage in the pre-merge checks, but it looks as if it's 
pre-existing breakage. See the comment-only change here that looks as if it 
exhibits similar breakage:

https://reviews.llvm.org/D127494


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

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

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

mboehme marked an inline comment as done.
mboehme added inline comments.



Comment at: clang/lib/Sema/ParsedAttr.cpp:228
+  switch (getParsedKind()) {
+  case AT_AddressSpace:
+  case AT_OpenCLPrivateAddressSpace:

rsmith wrote:
> I don't think this one really slides today, in the syntactic sense, prior to 
> this patch (and I guess the same is true for most of these). Given:
> 
> ```
> [[clang::address_space(5)]] int x;
> int [[clang::address_space(5)]] y;
> ```
> 
> ... we accept `x` and reject `y`, making it look like this is simply a 
> declaration attribute. Hm, but we accept:
> 
> ```
> int *[[clang::address_space(5)]] *z;
> ```
> 
> ... so it really *does* seem to behave like a type attribute sometimes?
> 
> OK, so this patch is doing two things:
> 
> 1) It's permitting the use of these attributes as type attributes, where they 
> were not consistently permitted previously.
> 2) It's deprecating / warning on uses of them as declaration attributes.
> 
> That means there's no way to write code that is compatible with both old and 
> new Clang and produces no warnings, but it's easy enough to turn the warning 
> flag off I suppose. The new behavior does seem much better than the old.
> I don't think this one really slides today, in the syntactic sense, prior to 
> this patch (and I guess the same is true for most of these). Given:
> 
> ```
> [[clang::address_space(5)]] int x;
> int [[clang::address_space(5)]] y;
> ```
> 
> ... we accept `x` and reject `y`, making it look like this is simply a 
> declaration attribute.

Yes, and we get the same behavior for all C++11 attributes.

This is because, today, Clang flat-out rejects _all_ C++11 attributes that are 
placed after a decl-specifier-seq:

From `ParseDeclarationSpecifiers()`:

```
// Reject C++11 attributes that appertain to decl specifiers as
// we don't support any C++11 attributes that appertain to decl
// specifiers. This also conforms to what g++ 4.8 is doing.
ProhibitCXX11Attributes(attrs, diag::err_attribute_not_type_attr);
```

https://github.com/llvm/llvm-project/blob/main/clang/lib/Parse/ParseDecl.cpp#L3182

So really what the "slides" terminology means is "is treated as appertaining to 
the decl-specifier-seq instead of the declaration".

> Hm, but we accept:
> 
> ```
> int *[[clang::address_space(5)]] *z;
> ```
> 
> ... so it really *does* seem to behave like a type attribute sometimes?

Yes, because Clang does allow C++11 attributes (including `address_space` in 
particular) on declarator chunks today -- it just doesn't allow them on the 
decl-specifier-seq. As an aside, this makes the error message we get when 
putting the attribute on the decl-specifier-seq today ("'address_space' 
attribute cannot be applied to types") pretty misleading.

> OK, so this patch is doing two things:
> 
> 1) It's permitting the use of these attributes as type attributes, where they 
> were not consistently permitted previously.
> 2) It's deprecating / warning on uses of them as declaration attributes.

Actually, 1) happens in https://reviews.llvm.org/D111548, which this change is 
based on.

I did things this way because

  - I want to introduce `annotate_type` first, so that this change can use it 
in tests as an example of a type attribute that doesn't "slide"
  - I wanted tests in https://reviews.llvm.org/D111548 to be able to use 
`annotate_type` in all places that it's intended for, including on the 
decl-specifier-seq, so I needed to open up that usage. (Actually, when I first 
authored https://reviews.llvm.org/D111548, I didn't even realize that this 
followup change would be coming.)

Is this confusing? I could instead do the following:

  - Move the change that allows C++11 attributes on the decl-specifier-seq from 
https://reviews.llvm.org/D111548 to here.
  - Move tests that use `annotate_type` on the decl-specifier-seq from 
https://reviews.llvm.org/D111548 to here.

Do you think that would be preferable?

> That means there's no way to write code that is compatible with both old and 
> new Clang and produces no warnings, but it's easy enough to turn the warning 
> flag off I suppose.

That's true -- and I think it's the best we can do. The behavior that Clang 
exhibits today is of course set in stone (error out if the attribute is placed 
on the decl-specifier-seq), and at the same time it seems obvious that we want 
to nudge people away from putting the attribute on the declaration, as it's 
really a type attribute. (Just repeating your analysis here essentially; I 
think we agree, and there's nothing to do here.)

I do think this makes an argument that the new diagnostic we're introducing 
should be a warning initially, not an error. Otherwise, there would be no way 
to use the C++11 spelling in a way that compiles (albeit maybe with warnings) 
both before and after this patch.

> The new behavior does seem much better than the old.





Comment at: 

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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.



Comment at: clang/lib/Parse/ParseStmt.cpp:242-246
+Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+GNUAttrs, );
   } else {
-Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, Attrs);
+Decl = ParseDeclaration(DeclaratorContext::Block, DeclEnd, CXX11Attrs,
+GNUAttrs);

mboehme wrote:
> rsmith wrote:
> > mboehme wrote:
> > > aaron.ballman wrote:
> > > > rsmith wrote:
> > > > > I think this is the only place where we're passing decl-specifier-seq 
> > > > > attributes into `ParseDeclaration`. There are only two possible cases 
> > > > > here:
> > > > > 
> > > > > 1) We have a simple-declaration, and can `ParseSimpleDeclaration` 
> > > > > directly.
> > > > > 2) We have a static-assert, using, or namespace alias declaration, 
> > > > > which don't permit attributes at all.
> > > > > 
> > > > > So I think we *could* simplify this so that decl-spec attributes are 
> > > > > never passed into `ParseDeclaration`:
> > > > > 
> > > > > * If the next token is `kw_static_assert`, `kw_using`, or 
> > > > > `kw_namespace`, then prohibit attributes and call `ParseDeclaration`.
> > > > > * Otherwise, call `ParseSimpleDeclaration` and pass in the attributes.
> > > > > * Remove the `DeclSpecAttrs` list from `ParseDeclaration`.
> > > > > 
> > > > > I'm not requesting a change here -- I'm not sure whether that's a net 
> > > > > improvement or not -- but it seems worth considering.
> > > > I think this is a good avenue to explore -- passing in two different 
> > > > attribute lists means someone will at some point get it wrong by 
> > > > accident, so only having one attribute list reduces the chances for 
> > > > bugs later. I don't imagine static assertions or namespaces will get 
> > > > leading attributes. However...
> > > > 
> > > > I think asm-declaration and using-directive are also a bit special -- 
> > > > they're allowed to have leading attributes: 
> > > > http://eel.is/c++draft/dcl.dcl#nt:asm-declaration and 
> > > > http://eel.is/c++draft/dcl.dcl#nt:using-directive
> > > > 
> > > > Do we also have to handle opaque-enum-declaration here? 
> > > > http://eel.is/c++draft/dcl.dcl#nt:block-declaration
> > > I investigated this, but I'm not sure it's a net win.
> > > 
> > > > * If the next token is `kw_static_assert`, `kw_using`, or 
> > > > `kw_namespace`, then prohibit attributes and call `ParseDeclaration`.
> > > 
> > > Or if the next token is `kw_inline` and the token after that is 
> > > `kw_namespace`...
> > > 
> > > I think going down this path would mean duplicating all of the case 
> > > distinctions in `ParseDeclaration()` -- and if we add more cases in the 
> > > future, we'd have to make sure to replicate them here. I think overall it 
> > > keeps the code simpler to accept the additional `DeclSpecAttrs` parameter.
> > > 
> > > > Do we also have to handle opaque-enum-declaration here? 
> > > > http://eel.is/c++draft/dcl.dcl#nt:block-declaration
> > > 
> > > A moot point, probably, but I believe this is handled by 
> > > `ParseEnumSpecifier()`, which is called from 
> > > `ParseDeclarationSpecifiers()`, so there would be no need to handle it 
> > > separately.
> > > > * If the next token is `kw_static_assert`, `kw_using`, or 
> > > > `kw_namespace`, then prohibit attributes and call `ParseDeclaration`.
> > > 
> > > Or if the next token is `kw_inline` and the token after that is 
> > > `kw_namespace`...
> > 
> > This function is parsing block declarations, so we don't have to handle 
> > `inline namespace` here because that can't appear as a block declaration. 
> > The only reason we need to handle `namespace` is to support namespace 
> > aliases (`namespace A = B;`), which are (perhaps surprisingly) allowed at 
> > block scope.
> > 
> > > I think going down this path would mean duplicating all of the case 
> > > distinctions in `ParseDeclaration()` -- and if we add more cases in the 
> > > future, we'd have to make sure to replicate them here. I think overall it 
> > > keeps the code simpler to accept the additional `DeclSpecAttrs` parameter.
> > 
> > It's not all the cases, only the block-declaration cases. If we wanted to 
> > directly follow the grammar, we could split a `ParseBlockDeclaration` 
> > function out of `ParseDeclaration`, but I guess there'd be so little left 
> > in `ParseDeclaration` that it wouldn't be worth it, and we'd still have to 
> > pass the `DeclSpecAttrs` parameter to `ParseBlockDeclaration`, so that 
> > doesn't save us anything. I suppose it'd also regress our diagnostic 
> > quality for declarations that aren't allowed at block scope. OK, I'm 
> > convinced :-)
> Just confirming: You're not asking me to do anything here, correct?
Yes, no action requested. Thanks for talking through this option with me! :)



Comment at: clang/lib/Sema/ParsedAttr.cpp:228
+  switch 

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

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

mboehme marked 8 inline comments as done.
mboehme added a comment.

I notice that https://reviews.llvm.org/D111548, which this change is based on, 
has some failing flang tests in the CI. I'm pretty sure these are unrelated and 
stem from the base revision that I happen to be based off. Before rebasing, 
however, I wanted to make sure all of the big comments have been resolved so 
that I don't introduce too much churn into the diffs.




Comment at: clang/include/clang/Sema/DeclSpec.h:1853-1856
   /// Attrs - Attributes.
   ParsedAttributes Attrs;
 
+  ParsedAttributes DeclarationAttrs;

aaron.ballman wrote:
> mboehme wrote:
> > aaron.ballman wrote:
> > > We should probably rename `Attrs` to be less generic and add some 
> > > comments to explain why there are two lists of parsed attributes.
> > I've added some comments to clarify the difference. I'm not sure what a 
> > good alternative name for `Attrs` would be though. The only obvious 
> > candidate that comes to mind is `DeclaratorAttrs`, but that seems pretty 
> > redundant given that this is a field of `Declarator`.
> > 
> > I think there is actually a good case for calling the first list simply 
> > `Attrs` because, unlike `DeclarationAttrs`, it applies directly to the 
> > `Declarator`. This distinction is amplified by the fact that, since your 
> > review, I've changed `DeclarationAttrs` to be a reference, not a by-value 
> > field.
> The new comment actually makes it less clear for me -- that says the 
> attributes are for the `DeclSpec` but they're actually for the `Declatator` 
> (there's no relationship between `Attrs` and `DS`).
> 
> Given that `DeclSpec` uses `Attrs` for its associated attributes, I think 
> it's okay to use `Attrs` here for the ones associated with the `Declarator`, 
> but the comment should be updated in that case.
Sorry, I don't know what I was thinking when I wrote `DeclSpec`. That should 
have read "Attributes attached to the declarator", and I've changed it 
accordingly.

I hope that with this change, this makes sense now?



Comment at: clang/include/clang/Sema/DeclSpec.h:1926-1928
+assert(llvm::all_of(DeclarationAttrs, [](const ParsedAttr ) {
+  return AL.isStandardAttributeSyntax();
+}));

rsmith wrote:
> rsmith wrote:
> > I think I recall seeing OpenMP pragma attributes being parsed as 
> > declaration attributes too. FIXME update this comment after review is 
> > complete
> Oops, forgot to circle back to this. I think I was wrong; please disregard :)
Just making sure I can disregard this comment? Please respond if there's 
something you'd like me to address here.



Comment at: clang/include/clang/Sema/ParsedAttr.h:664
+  /// This may only be called if isStandardAttributeSyntax() returns true.
+  bool slidesFromDeclToDeclSpec() const;
+

aaron.ballman wrote:
> Because this is now specific to standard attributes, and those don't "slide" 
> under normal circumstances, it might be nice to rename this method to make it 
> more clear that this should not be called normally. I don't have a strong 
> opinion on what a *good* name is, but even something like 
> `improperlySlidesFromDeclToDeclSpec()` would make me happy.
I've decided to add `LegacyBehavior` at the end -- WDYT?



Comment at: clang/lib/Parse/ParseDecl.cpp:1854
+ProhibitAttributes(DeclAttrs);
+ProhibitAttributes(OriginalDeclSpecAttrs);
 DeclEnd = Tok.getLocation();

aaron.ballman wrote:
> rsmith wrote:
> > This looks wrong to me (both before and after this patch; you've faithfully 
> > retained an incorrect behavior). If `DeclSpec` attributes aren't allowed, 
> > they should be diagnosed by `ParsedFreeStandingDeclSpec`. Before and after 
> > this patch, we'll diagnose attributes in a free-standing decl-specifier-seq 
> > if we happened to tentatively parse them, not if we happened to parse them 
> > in `ParseDeclarationSpecifiers`. For example:
> > 
> > ```
> > __attribute__(()) struct X; // DeclSpecAttrs will be empty, no error
> > void f() {
> >   __attribute(()) struct X; // DeclSpecAttrs will contain the attribute 
> > specifier, error
> > }
> > ```
> > 
> > Comparing to GCC's behavior for that example (which GCC silently accepts) 
> > and for this one:
> > 
> > ```
> > __attribute__((packed)) struct X; // GCC warns that packed has no meaning 
> > here, clang produces a high-quality warning.
> > void f() {
> >   __attribute((packed)) struct X; // GCC warns that packed has no meaning 
> > here, clang produces a bogus error.
> > }
> > ```
> > 
> > ... I think the right thing to do is to delete this call (along with 
> > `OriginalDeclSpecAttrs`). GNU decl-specifier attributes *are* apparently 
> > syntactically permitted here, but most (maybe even all?) GNU attributes 
> > don't have any meaning in this position.
> Good catch!
Thanks for pointing this out!

I've removed the `ParseAttributes(DeclSpecAttrs)`