[PATCH] D137979: parse: process GNU and standard attributes on top-level decls

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thank you for working on this, I know it was a slog! I think this is pretty close to ready, though you should definitely add a release note about the changes.



================
Comment at: clang/include/clang/Parse/Parser.h:1605-1606
   // C99 6.9: External Definitions.
   DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributes &Attrs,
+                                          ParsedAttributes &DeclSpecAttrs,
                                           ParsingDeclSpec *DS = nullptr);
----------------
Should we rename `Attrs` to `DeclaratorAttrs` or something along those lines, so it's easier for someone to distinguish which attributes go where? (Same suggestion applies throughout the review.)


================
Comment at: clang/lib/Parse/ParseObjc.cpp:67-69
+    for (const auto &PA : Attrs)
+      if (PA.isGNUAttribute())
+        Diag(Tok.getLocation(), diag::err_objc_unexpected_attr);
----------------
llvm::for_each?


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1997
 
+  ParsedAttributes EmptyDeclSpecAttrs(AttrFactory);
+
----------------
We can sink this down to the only place it's used in the function, right?


================
Comment at: clang/lib/Parse/Parser.cpp:735-737
+  ParsedAttributes DeclSpecAttrs(AttrFactory);
+  while (MaybeParseCXX11Attributes(attrs) ||
+         MaybeParseGNUAttributes(DeclSpecAttrs))
----------------
We probably could use some comments here explaining why we parse some into one container and others into another container.


================
Comment at: clang/lib/Parse/Parser.cpp:1096-1097
+    ParsingDeclSpec &DS, AccessSpecifier AS) {
+  DS.SetRangeStart(DeclSpecAttrs.Range.getBegin());
+  DS.SetRangeEnd(DeclSpecAttrs.Range.getEnd());
+  DS.takeAttributesFrom(DeclSpecAttrs);
----------------
How sure are you that this isn't overwriting existing range data that's potentially relevant? e.g., where declaration specifiers and attributes are mixed together such that the attribute range doesn't cover all of the declaration specifiers?


================
Comment at: clang/test/Parser/attr-order.cpp:20
 __attribute__((cdecl)) __declspec(dllexport) [[noreturn]] void f(); // expected-error {{an attribute list cannot appear here}}
-__attribute__((cdecl)) [[noreturn]] __declspec(dllexport) void g(); // expected-error {{an attribute list cannot appear here}}
+__attribute__((cdecl)) [[noreturn]] __declspec(dllexport) void g();
 
----------------
This makes me happy!!


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137979/new/

https://reviews.llvm.org/D137979