[clang][parser] Allow GNU attributes before namespace identifier

[ArcsinX]

Summary

GCC supports:

• namespace <gnu attributes> identifier
• namespace identifier <gnu attributes>

But clang supports only namespace identifier <gnu attributes> and diagnostics for namespace <gnu attributes> identifier case looks unclear:
Code:

namespace __attribute__((visibility("hidden"))) A
{
}

Diags:

test.cpp:1:49: error: expected identifier or '{'
namespace __attribute__((visibility("hidden"))) A
                                                ^
test.cpp:1:49: error: C++ requires a type specifier for all declarations
test.cpp:3:2: error: expected ';' after top level declarator
}

This patch adds support for namespace <gnu attributes> identifier and also forbids gnu attributes for nested namespaces (this already done for C++ attributes).

[aaron.ballman]

Oh wow, GCC has supported this for a long time; I wasn't aware of that. Thanks for this patch!

clang/lib/Parse/ParseDeclCXX.cpp

78

However, I don't think there's a reason we need this lambda -- it seems we can call MaybeParseGNUAttributes() instead, and get the attribute location from the ParsedAttributesWithRange object passed in.

clang/test/Parser/namespace-attributes.cpp

4

Another test case that I think we need is to test attribute parsing order between the different syntaxes. e.g.,

namespace __attribute__(()) [[]] A
{
}

namespace [[]] __attribute__(()) B
{
}

It's not clear to me that we want to match how GCC handles this case: https://godbolt.org/z/e1bjcEje6 but instead handle it more like how Clang handles it in structure declarations: https://godbolt.org/z/drGhb65ET.

[ArcsinX]

• get attributes location from ParsedAttributesWithRange object
• parse subsequent attributes of different kinds in a loop

[ArcsinX]

clang/lib/Parse/ParseDeclCXX.cpp

78

MaybeParseAttributes(PAKM_GNU | PAKM_CXX11, Attrs) parses different kind of attributes in a loop (e.g. struct __attribute__(()) [[]] [[]] __attribute__(()) S {}; is valid for clang), but we can't use it because we need to warn about c++11 attributes usage for namespace if c++ < 17. So, I added a lambda which works similar to MaybeParseAttributes().

Thus, now clang handles attributes for namespace like it handle attributes for structure.

[aaron.ballman]

LGTM! Might be worth adding a release note for it (does this close any bugs in the bug database? If so, it'd be worth mentioning those in the commit message and release note).

[ArcsinX]

Update release notes
Rebase

[ArcsinX]

In D121245#3380346, @aaron.ballman wrote:

LGTM! Might be worth adding a release note for it (does this close any bugs in the bug database? If so, it'd be worth mentioning those in the commit message and release note).

Thank you for review.

I failed to find any opened bugs related to this. I'm not sure how detailed information about this patch should be in release notes. Can you please take a look at ReleaseNotes.rst modifications?

[aaron.ballman]

In D121245#3382887, @ArcsinX wrote:

In D121245#3380346, @aaron.ballman wrote:

LGTM! Might be worth adding a release note for it (does this close any bugs in the bug database? If so, it'd be worth mentioning those in the commit message and release note).

Thank you for review.

I failed to find any opened bugs related to this. I'm not sure how detailed information about this patch should be in release notes. Can you please take a look at ReleaseNotes.rst modifications?

The release notes look great to me, thank you for adding them!

clang/docs/ReleaseNotes.rst

 --------------------------
 
 - Added support for parameter pack expansion in `clang::annotate`.
 
 - The ``overloadable`` attribute can now be written in all of the syntactic
   locations a declaration attribute may appear.
   This fixes `Issue 53805 <https://github.com/llvm/llvm-project/issues/53805>`_.
 
+- Improved namespace attributes handling:
+  - Handle GNU attributes before a namespace identifier and subsequent
+    attributes of different kinds.
+  - Emit error on GNU attributes for a nested namespace definition.
+
 Windows Support
 ---------------
 
 - Add support for MSVC-compatible ``/JMC``/``/JMC-`` flag in clang-cl (supports
   X86/X64/ARM/ARM64). ``/JMC`` could only be used when ``/Zi`` or ``/Z7`` is
   turned on. With this addition, clang-cl can be used in Visual Studio for the
   JustMyCode feature. Note, you may need to manually add ``/JMC`` as additional
   compile options in the Visual Studio since it currently assumes clang-cl does not support ``/JMC``.

clang/lib/Parse/ParseDeclCXX.cpp

Parser::DeclGroupPtrTy Parser::ParseNamespace(DeclaratorContext Context,
   }
 
   SourceLocation IdentLoc;
   IdentifierInfo *Ident = nullptr;
   InnerNamespaceInfoList ExtraNSs;
   SourceLocation FirstNestedInlineLoc;
 
   ParsedAttributesWithRange attrs(AttrFactory);
-  SourceLocation attrLoc;
+
+  auto ReadAttributes = [&] {
+    bool MoreToParse;
+    do {
+      MoreToParse = false;
+      if (Tok.is(tok::kw___attribute)) {
+        ParseGNUAttributes(attrs);
+        MoreToParse = true;
+      }
       if (getLangOpts().CPlusPlus11 && isCXX11AttributeSpecifier()) {
         Diag(Tok.getLocation(), getLangOpts().CPlusPlus17
                                     ? diag::warn_cxx14_compat_ns_enum_attribute
                                     : diag::ext_ns_enum_attribute)
             << 0 /*namespace*/;
-    attrLoc = Tok.getLocation();
         ParseCXX11Attributes(attrs);
+        MoreToParse = true;
       }
+    } while (MoreToParse);
+  };
+
+  ReadAttributes();
 
   if (Tok.is(tok::identifier)) {
     Ident = Tok.getIdentifierInfo();
     IdentLoc = ConsumeToken();  // eat the identifier.
     while (Tok.is(tok::coloncolon) &&
            (NextToken().is(tok::identifier) ||
             (NextToken().is(tok::kw_inline) &&
              GetLookAheadToken(2).is(tok::identifier)))) {
while (Tok.is(tok::coloncolon) &&
 
       Info.Ident = Tok.getIdentifierInfo();
       Info.IdentLoc = ConsumeToken();
 
       ExtraNSs.push_back(Info);
     }
   }
 
+  ReadAttributes();
+
+  SourceLocation attrLoc = attrs.Range.getBegin();
+
   // A nested namespace definition cannot have attributes.
   if (!ExtraNSs.empty() && attrLoc.isValid())
     Diag(attrLoc, diag::err_unexpected_nested_namespace_attribute);
 
-  // Read label attributes, if present.
-  if (Tok.is(tok::kw___attribute)) {
-    attrLoc = Tok.getLocation();
-    ParseGNUAttributes(attrs);
-  }
-
   if (Tok.is(tok::equal)) {
     if (!Ident) {
       Diag(Tok, diag::err_expected) << tok::identifier;
       // Skip to end of the definition and eat the ';'.
       SkipUntil(tok::semi);
       return nullptr;
     }
     if (attrLoc.isValid())

clang/test/Parser/namespace-attributes.cpp

+// RUN: %clang_cc1 -std=c++17 -verify %s
+
+namespace __attribute__(()) A
+{
+}
+
+namespace A __attribute__(())
+{
+}
+
+namespace __attribute__(()) [[]] A
+{
+}
+
+namespace [[]] __attribute__(()) A
+{
+}
+
+namespace A __attribute__(()) [[]]
+{
+}
+
+namespace A [[]] __attribute__(())
+{
+}
+
+namespace [[]] A __attribute__(())
+{
+}
+
+namespace __attribute__(()) A [[]]
+{
+}
+
+namespace A::B __attribute__(()) // expected-error{{attributes cannot be specified on a nested namespace definition}}
+{
+}
+
+namespace __attribute__(()) A::B // expected-error{{attributes cannot be specified on a nested namespace definition}}
+{
+}