rsmith 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()));
----------------
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D51329/new/
https://reviews.llvm.org/D51329
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
