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

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Aug 31 05:32:47 PDT 2018

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.

  rC Clang


More information about the cfe-commits mailing list