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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.

You also need to update the various other places that create `AttributedType`s (eg, template instantiation, ASTImporter, deserialization) to pass in the macro name.  You can try removing the default argument from `ASTContext::getAttributedType` to find other places that might need updating.



================
Comment at: include/clang/AST/Type.h:4318-4323
+  // If the attribute this type holds is address_space and this attribute was
+  // declared as part of a macro, we store the macro identifier information.
+  // This will be used for printing the macro name instead of
+  // `__attribute__((address_space(n)))` in diagnostics relating to differing
+  // address_spaces between types.
+  IdentifierInfo *AddressSpaceMacroII;
----------------
Please keep this general, there's nothing address-space-specific here.


================
Comment at: include/clang/AST/Type.h:4343-4344
   QualType getEquivalentType() const { return EquivalentType; }
+  IdentifierInfo *getAddressSpaceMacroII() const { return AddressSpaceMacroII; }
+  bool hasAddressSpaceMacroII() const { return AddressSpaceMacroII != nullptr; }
 
----------------
Likewise.


================
Comment at: include/clang/Sema/ParsedAttr.h:538-542
+  void setMacroII(IdentifierInfo *MacroName) { MacroII = MacroName; }
+
+  bool hasMacroII() const { return MacroII != nullptr; }
+
+  IdentifierInfo *getMacroII() const { return MacroII; }
----------------
Please use `MacroIdentifier` or `MacroName` in these function names; abbreviations such as `II` should only be used in entities with a relatively small scope, not in public member functions (here and in `AttributedType).

A documentation comment on these would also be useful.


================
Comment at: lib/AST/TypePrinter.cpp:1370
+
+    // Remove the underlying address space so it won't be printed.
+    SplitQualType SplitTy = T->getModifiedType().split();
----------------
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.


================
Comment at: lib/Parse/ParseDecl.cpp:169-170
   assert(Tok.is(tok::kw___attribute) && "Not a GNU attribute list!");
+  unsigned OldNumAttrs = attrs.size();
+  Token AttrTok = Tok;
 
----------------
This is not right: the loop below parses multiple `__attribute__`s and you should track each one separately...


================
Comment at: lib/Parse/ParseDecl.cpp:173
   while (Tok.is(tok::kw___attribute)) {
     ConsumeToken();
     if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen_after,
----------------
... by grabbing the location of the `__attribute__` token here.


================
Comment at: lib/Parse/ParseDecl.cpp:244-252
+    // If this was declared in a macro, attatch the macro IdentifierInfo to the
+    // parsed attribute.
+    for (const auto &MacroPair : PP.getAttributeMacros()) {
+      if (SourceLocInSourceRange(AttrTok.getLocation(), MacroPair.first,
+                                 PP.getSourceManager())) {
+        ApplyMacroIIToParsedAttrs(attrs, NumParsedAttrs, MacroPair.second);
+        break;
----------------
You shouldn't do this if `NumParsedAttrs != 1`. We're only looking for a macro that exactly covers one attribute.

(Also, your computation of the number of attributes in the attribute list is not correct in the presence of late-parsed attributes.)


================
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;
+      }
+    }
----------------
>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.


Repository:
  rC Clang

https://reviews.llvm.org/D51329