[PATCH] D51329: [Attribute/Diagnostics] Print macro if definition is an attribute declaration

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:257-262
+      bool AttrStartIsInMacro =
+          (StartLoc.isMacroID() && Lexer::isAtStartOfMacroExpansion(
+                                       StartLoc, SrcMgr, PP.getLangOpts()));
+      bool AttrEndIsInMacro =
+          (EndLoc.isMacroID() &&
+           Lexer::isAtEndOfMacroExpansion(EndLoc, SrcMgr, PP.getLangOpts()));
----------------
rsmith wrote:
> leonardchan wrote:
> > rsmith wrote:
> > > leonardchan wrote:
> > > > rsmith wrote:
> > > > > I think these checks need to be moved to the last loop of `FindLocsWithCommonFileID`. Otherwise for a case like:
> > > > > 
> > > > > ```
> > > > > #define THING \
> > > > >   int *p = nullptr;
> > > > >   AS1 int *q = nullptr;
> > > > > 
> > > > > THING
> > > > > ```
> > > > > 
> > > > > ... `FindLocsWithCommonFileID` will pick out the `THING` macro and return the range from the `int` token to the second semicolon, and the checks here will fail. Instead, we should pick out the inner `AS1` expansion, because it's the outermost macro expansion that starts with `StartLoc` and ends with `EndLoc`.
> > > > Moved, although this doesn't appear to fix this case. On closer inspection, when generating the vector of starting locations, the expansion location for `AS1` doesn't seem to be returned by `getExpansionLocStart`. It goes straight from the location of the `__attribute__` behind `AS1` to `THING` and skips `AS1`. I found this out by just dumping `StartLoc` on every iteration.
> > > > 
> > > > The only way I can generate the location for `AS1` in `THING` is by also watching for the spelling location of each expansion, but this SourceLoc is not a macro ID and would not fulfill these last 2 checks. Is this intended? If it's a bug I don't mind addressing it if you point me in the right direction to start.
> > > Right, sorry, mistake on my part. To deal with this, you need to call `isAt{Start,End}OfMacroExpansion` while collecting `StartLocs` and `EndLocs`, and stop once you hit a location where they return `false`. (That would mean that you never step from the location of `AS1` out to the location of `THING`.)
> > Hmm. So I still run into the problem of none of the locations being at the start or end of the macro expansion. In your example, I only find 2 source locations at all. Let's say I have:
> > 
> > ```
> > 1 #define AS1 __attribute__((address_space(1)))
> > 2
> > 3 int main() {
> > 4 #define THING \
> > 5   int *p = 0; \
> > 6   AS1 int *q = p;
> > 7
> > 8   THING;
> > 9 }
> > ```
> > 
> > I only see the following expansion source locations:
> > 
> > ```
> > /usr/local/google/home/leonardchan/misc/test.c:8:3 <Spelling=/usr/local/google/home/leonardchan/misc/test.c:1:13>
> > /usr/local/google/home/leonardchan/misc/test.c:8:3 <Spelling=/usr/local/google/home/leonardchan/misc/test.c:6:3>
> > ```
> > 
> > And it seems none of them return true for `isAtStartOfMacroExpansion` since the `__attribute__` keyword isn't the first token of `THING`. I imagine stopping early would work if the 2nd expansion location instead referred to `AS1` (on line 6 col 3), but it doesn't seem to be the case here.
> > 
> > I can also see similar results for other examples where the macro is nested but does not encapsulate the whole macro:
> > 
> > ```
> > #define THING2 int AS1 *q2 = p2;
> > int *p2;
> > THING2;  // Error doesn't print AS1
> > ```
> > 
> > For our case, it seems like the correct thing to do is to get the spelling location and default to it if the macro itself doesn't contain the whole attribute. I updated and renamed this function to account for this and we can now see `AS1` printed. Also added test cases for this.
> > 
> > For cases like `#define AS2 AS1`, `AS1` is still printed, but this is intended since `AS1` is more immediate of an expansion than `AS2`.
> I'd like to get some (correct) form of this landed, so I wonder if we can do something much simpler for now. Can we just check that the first token of the attribute-specifier `isAtStartOfMacroExpansion` and the last token `isAtEndOfMacroExpansion` and that they're from the same expansion of the same (object-like) macro? That won't find the outermost such macro, nor will it deal with cases where `__attribute__` or the last `)` was itself generated by a macro, but it should not have any false positives.
No problem. Done, and our original test cases still pass.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D51329/new/

https://reviews.llvm.org/D51329