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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: lib/AST/TypePrinter.cpp:1370
+
+    // Remove the underlying address space so it won't be printed.
+    SplitQualType SplitTy = T->getModifiedType().split();
----------------
rsmith wrote:
> leonardchan wrote:
> > rsmith wrote:
> > > leonardchan wrote:
> > > > rsmith wrote:
> > > > > This is unnecessary; just print the modified type here. (The modified type by definition does not have the attribute applied to it.)
> > > > When you say the modified type, do you mean just the type without it's qualifiers? I wasn't sure if removing all the qualifiers would suffice since there were also other  non-address_space qualifiers that could be printed.
> > > I mean `T->getModifiedType()`, which tracks what the type was before the attribute was applied.
> > Oh, I understand that you meant `T->getModifiedType()`. This is a special case when printing the `address_space` since even though the attribute is applied and printed here, when we reach the qualifiers of the modified type, the address space will still be printed unless we remove it here.
> > 
> > I'm not sure if there's a better way to do this though.
> If the address space qualifiers are present on the modified type, then that's a bug in the creation of the `AttributedType`. And indeed looking at r340765, I see we are passing the wrong type in as the modified type when creating the `AttributedType`. Please fix that, and then just use `getModifiedType` here.
Making another patch for this.


================
Comment at: lib/Lex/PPDirectives.cpp:2584
+// one token for the attribute itself.
+static constexpr unsigned kMinAttrTokens = 6;
+
----------------
aaron.ballman wrote:
> 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).
See comment below


================
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.
----------------
aaron.ballman wrote:
> Why does this need to be specific to GNU-style spellings?
Similar to the `noderef` patch I sent out, this is initially only meant for GNU-style attributes but can be expanded on later.

The main goal for our side was to change the diagnostics relating to address_spaces to instead print the macro if the attribute was defined in a macro. This was initially discussed in http://lists.llvm.org/pipermail/cfe-dev/2018-August/058702.html, but eventually decided on not using a pragma and instead redirecting diagnostics (http://lists.llvm.org/pipermail/cfe-dev/2018-August/058732.html).

Essentially, this was meant specifically for just the `address_space` attribute, which I believe only has GNU-style support, but Richard Smith thought that this would be useful to apply to other attributes. For our use case alone, we just need it to work for GNU-style, but we can expand on this afterwards for other attribute 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;
----------------
aaron.ballman wrote:
> 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)]]`.
Removed code relating to the processor since we don't need it anymore


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


================
Comment at: lib/Parse/ParseDecl.cpp:119
+  assert(attrs.size() >= N);
+  for (unsigned i = attrs.size() - N; i < N; ++i)
+    attrs[i].setMacroII(MacroII);
----------------
aaron.ballman wrote:
> You can use a range-based for loop here instead.
The condition should instead be `i < attrs.size()`, sorry.

I don't know if `ParsedAttributesView` has a reverse iterator though or I can start with an iterator in the middle of the view.


================
Comment at: lib/Parse/ParseDecl.cpp:246-252
+    for (const auto &MacroPair : PP.getAttributeMacros()) {
+      if (SourceLocInSourceRange(AttrTok.getLocation(), MacroPair.first,
+                                 PP.getSourceManager())) {
+        ApplyMacroIIToParsedAttrs(attrs, NumParsedAttrs, MacroPair.second);
+        break;
+      }
+    }
----------------
rsmith wrote:
> From my earlier comment: "look through the source location information for the outermost macro expansion that exactly covers the token sequence in question, and that's your corresponding macro name. (Note that we don't need to check what the expansion of the macro is, nor track what attribute-like macros have been defined, to support this approach.)"
> 
> We should not be tracking a list of attribute-like macros in the preprocessor. Doing so is unnecessary and problematic for various reasons.
Removed the preprocessor code


Repository:
  rC Clang

https://reviews.llvm.org/D51329