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

[Saleem Abdulrasool via Phabricator via cfe-commits]

compnerd marked 5 inline comments as done.
compnerd added inline comments.


================
Comment at: clang/include/clang/Parse/Parser.h:1605-1606
   // C99 6.9: External Definitions.
   DeclGroupPtrTy ParseExternalDeclaration(ParsedAttributes &Attrs,
+                                          ParsedAttributes &DeclSpecAttrs,
                                           ParsingDeclSpec *DS = nullptr);
----------------
aaron.ballman wrote:
> 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.)
Yes, `DeclAttrs` would be much nicer, but I wanted to get everything right before doing that and then forgot.  Thanks for the reminder.


================
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);
----------------
aaron.ballman wrote:
> 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?
I don't believe it can - the range hasn't been setup yet when coming into this function, only the storage has been allocated.  This is the first place where we are initializing the source range.  If there are other unstated invariants, it would be nice to have some sort of assertion for them.


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

https://reviews.llvm.org/D137979