[clang][parser] Unify rejecting (non) decl stmt with gnu attributes

[tbaeder]

Summary

I randomly noticed this comment (introduced in a3e01cf822f7415337e5424af3c6f4c94a12c1b9 from @rsmith), which is now incorrect.

It suggests that decl statements and others should be handled the same here.

I'm just posting this since it seems correct (according to the comment from 2013), but I'm not sure if the new diagnostic in the test case is really better than the old one.

If the old one is preferred, I can just reword the comment instead.

[aaron.ballman]

I think the behavior is correct, but I do think the original diagnostic was somewhat better -- an attribute list *can* appear there, if you're lucky. Of course, the old diagnostic isn't great either because adding a ; after the attribute isn't going to improve the situation for the user. So I think I'm actually fine either way, but maybe @rsmith has a preference.

[tbaeder]

Hey Aaron, it's been a while. Do you have a suggestion on how to proceed if @rsmith has no opinion on it?

[aaron.ballman]

In D99936#2694178, @tbaeder wrote:

Hey Aaron, it's been a while. Do you have a suggestion on how to proceed if @rsmith has no opinion on it?

I think the behavior in the patch is okay as-is, so LGTM, thank you!

clang/lib/Parse/ParseStmt.cpp

if (Tok.is(tok::kw___attribute)) {
     // semicolon, to disambiguate label attributes from attributes on a labeled
     // declaration.
     //
     // This doesn't quite match what GCC does; if the attribute list is empty
     // and followed by a semicolon, GCC will reject (it appears to parse the
     // attributes as part of a statement in that case). That looks like a bug.
     if (!getLangOpts().CPlusPlus || Tok.is(tok::semi))
       attrs.takeAllFrom(TempAttrs);
-    else if (isDeclarationStatement()) {
+    else {
       StmtVector Stmts;
-      // FIXME: We should do this whether or not we have a declaration
-      // statement, but that doesn't work correctly (because ProhibitAttributes
-      // can't handle GNU attributes), so only call it in the one case where
-      // GNU attributes are allowed.
       SubStmt = ParseStatementOrDeclarationAfterAttributes(Stmts, StmtCtx,
                                                            nullptr, TempAttrs);
       if (!TempAttrs.empty() && !SubStmt.isInvalid())
         SubStmt = Actions.ActOnAttributedStmt(TempAttrs, SubStmt.get());
-    } else {
-      Diag(Tok, diag::err_expected_after) << "__attribute__" << tok::semi;
     }
   }
 
   // If we've not parsed a statement yet, parse one now.
   if (!SubStmt.isInvalid() && !SubStmt.isUsable())
     SubStmt = ParseStatement(nullptr, StmtCtx);
 
   // Broken substmt shouldn't prevent the label from being added to the AST.

clang/test/SemaCXX/warn-unused-label-error.cpp

 // RUN: %clang_cc1 -fsyntax-only -Wunused-variable -Wunused-label -verify %s
 
 static int unused_local_static;
 
 namespace PR8455 {
   void f() {
     A: // expected-warning {{unused label 'A'}}
       __attribute__((unused)) int i; // attribute applies to variable
     B: // attribute applies to label
       __attribute__((unused)); int j; // expected-warning {{unused variable 'j'}}
   }
 
   void g() {
     C: // unused label 'C' will not appear here because an error has occurred
-      __attribute__((unused))
-      #pragma weak unused_local_static  // expected-error {{expected ';' after __attribute__}}
+      __attribute__((unused)) // expected-error {{an attribute list cannot appear here}}
+      #pragma weak unused_local_static
       ;
   }
 
   void h() {
   D:
 #pragma weak unused_local_static
     __attribute__((unused)) // expected-error {{'unused' attribute cannot be applied to a statement}}
     ;
   }
 }