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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
