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

[Roland McGrath via Phabricator via cfe-commits]

mcgrathr added inline comments.


================
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;
----------------
rsmith wrote:
> leonardchan wrote:
> > rsmith wrote:
> > > 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.)
> > One of the things we would like is for this to cover more than one attribute in the attribute list since in sparse, `address_space` is sometimes used with `noderef`.
> > 
> > So given `# define __user __attribute__((noderef, address_space(1)))`, `__user` would be saved into the `ParsedAttr` made for `noderef` and `address_space`.
> > 
> > What would be the appropriate way to track newly added attributes into the `ParsedAttributes`, including late-parsed attributes?
> > One of the things we would like is for this to cover more than one attribute in the attribute list since in sparse, `address_space` is sometimes used with `noderef`.
> 
> Hold on, this is a new requirement compared to what we'd previously discussed (giving a name to an address space). How important is this use case to you?
> 
> I don't think it's a reasonable AST model to assign a macro identifier to an `AttributedType` if the macro defines multiple attributes. If you really want to handle that use case, I think that an identifier on the `AttributedType` is the wrong way to model it, and we should instead be creating a new type sugar node representing a type decoration written as a macro.
> 
> Assuming you want to go ahead with the current patch direction (at least in the short term), please add the "only one attribute in the macro" check.
A single macro that uses multiple attributes is the central use case for us.
It would be fine if it's constrained to a single __attribute__ clause (or [[...]] clause) with the attributes comma-separated, as in __attribute__((foo, bar)) as opposed to two separate __attribute__((foo)) __attribute__((bar)) in the macro, if that matters.  It's even fine if it's constrained to the macro being nothing but the __attribute__((foo, bar)) clause (aside from whitespace and comments).

Leo can decide how he wants to proceed as far as incremental implementation.
But we won't have a real-world use for the feature only covering a single attribute.
We'll start using when it can cover the cases like the Linux __user and __iomem examples (address_space + noderef).


Repository:
  rC Clang

https://reviews.llvm.org/D51329