[PATCH] D51329: [Attribute/Diagnostics] Print macro instead of whole attribute for address_space

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/Type.h:4343
   QualType getEquivalentType() const { return EquivalentType; }
+  IdentifierInfo *getAddressSpaceMacroII() const { return AddressSpaceMacroII; }
+  bool hasAddressSpaceMacroII() const { return AddressSpaceMacroII != nullptr; }
----------------
rsmith wrote:
> Likewise.
Given that this function is `const`, I think the returned pointer should be as well.


================
Comment at: lib/Lex/PPDirectives.cpp:2584
+// one token for the attribute itself.
+static constexpr unsigned kMinAttrTokens = 6;
+
----------------
This count is specific to GNU spellings. For instance, a C++ spelling might have 5 tokens (two square brackets, attribute-token, two more square brackets) or more and a declspec spelling might have 4 tokens (__declspec keyword, paren, attribute token, paren).


================
Comment at: lib/Lex/PPDirectives.cpp:2586-2588
+/// This only catches macros whose whole definition is an attribute. That is, it
+/// starts with the attribute keyword and 2 opening parentheses, and ends with
+/// the 2 closing parentheses.
----------------
Why does this need to be specific to GNU-style spellings?


================
Comment at: lib/Lex/PPDirectives.cpp:2720-2721
 
+  // If the macro is an attribute that contains address_space(), save this for
+  // diagnosing later.
+  SourceRange Range;
----------------
This should not be specific to `address_space`, but even if it were, this is wrong as it can be spelled `[[clang::address_space(0)]]`.


================
Comment at: lib/Parse/ParseDecl.cpp:116
+/// they were defined in.
+static void ApplyMacroIIToParsedAttrs(ParsedAttributes &attrs, unsigned N,
+                                      IdentifierInfo *MacroII) {
----------------
`attrs` doesn't meet the usual naming conventions. Same with `i` below.


================
Comment at: lib/Parse/ParseDecl.cpp:119
+  assert(attrs.size() >= N);
+  for (unsigned i = attrs.size() - N; i < N; ++i)
+    attrs[i].setMacroII(MacroII);
----------------
You can use a range-based for loop here instead.


Repository:
  rC Clang

https://reviews.llvm.org/D51329