Require commas between double square bracket attributes.

Clang currently has a bug where it allows you to write [[foo bar]] and
both attributes are silently accepted. This patch corrects the comma
parsing rules for such attributes and handles the test case fallout, as
a few tests were accidentally doing this.

diff --git a/clang/lib/Parse/ParseDeclCXX.cpp b/clang/lib/Parse/ParseDeclCXX.cpp
index 4980363..e1d29f5 100644 (file)
--- a/clang/lib/Parse/ParseDeclCXX.cpp
+++ b/clang/lib/Parse/ParseDeclCXX.cpp
@@ -4213,10 +4213,21 @@ void Parser::ParseCXX11AttributeSpecifier(ParsedAttributes &attrs,
 
   llvm::SmallDenseMap<IdentifierInfo*, SourceLocation, 4> SeenAttrs;
 
+  bool AttrParsed = false;
   while (Tok.isNot(tok::r_square)) {
-    // attribute not present
-    if (TryConsumeToken(tok::comma))
-      continue;
+    if (AttrParsed) {
+      // If we parsed an attribute, a comma is required before parsing any
+      // additional attributes.
+      if (ExpectAndConsume(tok::comma)) {
+        SkipUntil(tok::r_square, StopAtSemi | StopBeforeMatch);
+        continue;
+      }
+      AttrParsed = false;
+    }
+
+    // Eat all remaining superfluous commas before parsing the next attribute.
+    while (TryConsumeToken(tok::comma))
+      ;
 
     SourceLocation ScopeLoc, AttrLoc;
     IdentifierInfo *ScopeName = nullptr, *AttrName = nullptr;
@@ -4250,7 +4261,6 @@ void Parser::ParseCXX11AttributeSpecifier(ParsedAttributes &attrs,
     }
 
     bool StandardAttr = IsBuiltInOrStandardCXX11Attribute(AttrName, ScopeName);
-    bool AttrParsed = false;
 
     if (StandardAttr &&
         !SeenAttrs.insert(std::make_pair(AttrName, AttrLoc)).second)
@@ -4262,12 +4272,14 @@ void Parser::ParseCXX11AttributeSpecifier(ParsedAttributes &attrs,
       AttrParsed = ParseCXX11AttributeArgs(AttrName, AttrLoc, attrs, endLoc,
                                            ScopeName, ScopeLoc);
 
-    if (!AttrParsed)
+    if (!AttrParsed) {
       attrs.addNew(
           AttrName,
           SourceRange(ScopeLoc.isValid() ? ScopeLoc : AttrLoc, AttrLoc),
           ScopeName, ScopeLoc, nullptr, 0,
           getLangOpts().CPlusPlus ? ParsedAttr::AS_CXX11 : ParsedAttr::AS_C2x);
+      AttrParsed = true;
+    }
 
     if (TryConsumeToken(tok::ellipsis))
       Diag(Tok, diag::err_cxx11_attribute_forbids_ellipsis)

diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
index 4591195..982f18f 100644 (file)
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.nodiscard/p1.cpp
@@ -1,7 +1,7 @@
 // RUN: %clang_cc1 -fsyntax-only -std=c++2a -verify %s
 
 struct [[nodiscard]] S1 {}; // ok
-struct [[nodiscard nodiscard]] S2 {}; // expected-error {{attribute 'nodiscard' cannot appear multiple times in an attribute specifier}}
+struct [[nodiscard, nodiscard]] S2 {}; // expected-error {{attribute 'nodiscard' cannot appear multiple times in an attribute specifier}}
 struct [[nodiscard("Wrong")]] S3 {};
 
 [[nodiscard]] int f();

diff --git a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p1.cpp b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p1.cpp
index 8da2ca7..6c9b8a7 100644 (file)
--- a/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p1.cpp
+++ b/clang/test/CXX/dcl.dcl/dcl.attr/dcl.attr.unused/p1.cpp
@@ -1,5 +1,5 @@
 // RUN: %clang_cc1 -fsyntax-only -Wunused -std=c++1z -verify %s
 
 struct [[maybe_unused]] S1 {}; // ok
-struct [[maybe_unused maybe_unused]] S2 {}; // expected-error {{attribute 'maybe_unused' cannot appear multiple times in an attribute specifier}}
+struct [[maybe_unused, maybe_unused]] S2 {}; // expected-error {{attribute 'maybe_unused' cannot appear multiple times in an attribute specifier}}
 struct [[maybe_unused("Wrong")]] S3 {}; // expected-error {{'maybe_unused' cannot have an argument list}}

diff --git a/clang/test/Parser/c2x-attributes.c b/clang/test/Parser/c2x-attributes.c
index 393506e..48bb197 100644 (file)
--- a/clang/test/Parser/c2x-attributes.c
+++ b/clang/test/Parser/c2x-attributes.c
@@ -16,6 +16,13 @@ enum { [[]] Six }; // expected-error {{expected identifier}}
 // FIXME: this diagnostic can be improved.
 enum E3 [[]] { Seven }; // expected-error {{expected identifier or '('}}
 
+[[,,,,,]] int Commas1; // ok
+[[,, maybe_unused]] int Commas2; // ok
+[[maybe_unused,,,]] int Commas3; // ok
+[[,,maybe_unused,]] int Commas4; // ok
+[[foo bar]] int NoComma; // expected-error {{expected ','}} \
+                         // expected-warning {{unknown attribute 'foo' ignored}}
+
 struct [[]] S1 {
   int i [[]];
   int [[]] j;
@@ -117,8 +124,7 @@ void test_asm(void) {
 }
 
 // Do not allow 'using' to introduce vendor attribute namespaces.
-[[using vendor: attr1, attr2]] void f14(void); // expected-error {{expected ']'}} \
-                                               // expected-warning {{unknown attribute 'vendor' ignored}} \
+[[using vendor: attr1, attr2]] void f14(void); // expected-error {{expected ','}} \
                                                // expected-warning {{unknown attribute 'using' ignored}}
 
 struct [[]] S4 *s; // expected-error {{an attribute list cannot appear here}}

diff --git a/clang/test/Parser/cxx-attributes.cpp b/clang/test/Parser/cxx-attributes.cpp
index 53b098b..2b874e4 100644 (file)
--- a/clang/test/Parser/cxx-attributes.cpp
+++ b/clang/test/Parser/cxx-attributes.cpp
@@ -34,3 +34,10 @@ void fn() {
   int (*__attribute__((attr(i[1]))) pi);  // expected-warning{{unknown attribute 'attr' ignored}}
   pi = &i[0];
 }
+
+[[,,,,,]] int Commas1; // ok
+[[,, maybe_unused]] int Commas2; // ok
+[[maybe_unused,,,]] int Commas3; // ok
+[[,,maybe_unused,]] int Commas4; // ok
+[[foo bar]] int NoComma; // expected-error {{expected ','}} \
+                         // expected-warning {{unknown attribute 'foo' ignored}}

diff --git a/clang/test/Parser/pragma-attribute.cpp b/clang/test/Parser/pragma-attribute.cpp
index 4644bc1..d51f815 100644 (file)
--- a/clang/test/Parser/pragma-attribute.cpp
+++ b/clang/test/Parser/pragma-attribute.cpp
@@ -165,9 +165,9 @@ _Pragma("clang attribute pop");
 
 #pragma clang attribute push ([[]], apply_to = function) // A noop
 
-#pragma clang attribute push ([[noreturn ""]], apply_to=function) // expected-error {{expected ']'}}
+#pragma clang attribute push ([[noreturn ""]], apply_to=function) // expected-error {{expected ','}}
 #pragma clang attribute pop
-#pragma clang attribute push ([[noreturn 42]]) // expected-error {{expected ']'}} expected-error {{expected ','}}
+#pragma clang attribute push ([[noreturn 42]]) // expected-error 2 {{expected ','}}
 
 #pragma clang attribute push(__attribute__, apply_to=function) // expected-error {{expected '(' after 'attribute'}}
 #pragma clang attribute push(__attribute__(), apply_to=function) // expected-error {{expected '(' after '('}}

diff --git a/clang/test/Sema/c2x-maybe_unused-errors.c b/clang/test/Sema/c2x-maybe_unused-errors.c
index 39ec2da..5d3eb4d 100644 (file)
--- a/clang/test/Sema/c2x-maybe_unused-errors.c
+++ b/clang/test/Sema/c2x-maybe_unused-errors.c
@@ -3,7 +3,7 @@
 struct [[maybe_unused]] S1 { // ok
   int a [[maybe_unused]];
 };
-struct [[maybe_unused maybe_unused]] S2 { // expected-error {{attribute 'maybe_unused' cannot appear multiple times in an attribute specifier}}
+struct [[maybe_unused, maybe_unused]] S2 { // expected-error {{attribute 'maybe_unused' cannot appear multiple times in an attribute specifier}}
   int a;
 };
 struct [[maybe_unused("Wrong")]] S3 { // expected-error {{'maybe_unused' cannot have an argument list}}

diff --git a/clang/test/Sema/c2x-nodiscard.c b/clang/test/Sema/c2x-nodiscard.c
index cf1f081..f62ba27 100644 (file)
--- a/clang/test/Sema/c2x-nodiscard.c
+++ b/clang/test/Sema/c2x-nodiscard.c
@@ -3,7 +3,7 @@
 struct [[nodiscard]] S1 { // ok
   int i;
 };
-struct [[nodiscard nodiscard]] S2 { // expected-error {{attribute 'nodiscard' cannot appear multiple times in an attribute specifier}}
+struct [[nodiscard, nodiscard]] S2 { // expected-error {{attribute 'nodiscard' cannot appear multiple times in an attribute specifier}}
   int i;
 };
 struct [[nodiscard("Wrong")]] S3 { // FIXME: may need an extension warning.