sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.
Awesome, thanks for fixing this!
================
Comment at: clang-tools-extra/clangd/Selection.cpp:261
+ // consider it selected.
+ if (!SeenMacroCalls.insert(ArgStart).second) {
+ return NoTokens;
----------------
nridge wrote:
> sammccall wrote:
> > sammccall wrote:
> > > Given the following program:
> > > ```
> > > #define SQUARE(x) x * x;
> > > int four = [[SQUARE(2)]];
> > > ```
> > > We're going to now report the binary operator and one of the operands as
> > > selected and not the other, which doesn't seem desirable.
> > >
> > > I think we want to accept macro-selected || arg-selected, so probably
> > > doing the current "non-argument macro expansion" first unconditionally or
> > > factoring it out into a function.
> > >
> > > This will change the behavior of `int four = [[SQUARE]](2)` to consider
> > > the literal children selected too, I think this is fine.
> > I don't think it's a good idea to add hidden state and side-effects to
> > testChunk() - it breaks a lot of assumptions that help reason about the
> > code, and using `mutable` hides the violation of them.
> > (And a possible source of bugs - this is first in traversal order rather
> > than first in source order - these are mostly but IIRC not always the same).
> >
> > Instead I think you can do this statelessly: from the top-level spelling
> > location, walk down with `SM.getMacroArgExpandedLocation` until you hit the
> > target FileID (this is the first-expansion of first-expansion of
> > first-expansion...) or the FileID stops changing (you've reached the
> > innermost macro invocation, and your target location was on a different
> > branch).
> I agree that adding state is not great. I thought it was icky as I was
> writing it, I just couldn't think of an alternative. Thank you for suggesting
> one!
>
> I implemented what you suggested, and it seems to work. I did want to ask a
> clarifying question to make sure I understand correctly: when an argument
> occurs multiple times in a macro exapnsion, the occurrences will have
> distinct `FileID`s (as opposed just different offsets in the same macro
> `FileID`)?
That's right. My mental model is:
- a TU is a sequence of expanded (i.e. post-PP) tokens
- a FileID always corresponds to a single subrange of these expanded tokens...
- ...with holes in it for child FileIDs. Parents/children nest properly.
One of the things I really like about the syntax::TokenBuffer API is that it
exposes this structure.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:166
+ if (SM.getFileID(Next) == SM.getFileID(Prev)) {
+ break;
+ }
----------------
nit: return false directly?
================
Comment at: clang-tools-extra/clangd/Selection.cpp:287
if (SM.getFileID(ArgStart) == SelFile) {
- SourceLocation ArgEnd = SM.getTopMacroCallerLoc(Batch.back().location());
- return testTokenRange(SM.getFileOffset(ArgStart),
- SM.getFileOffset(ArgEnd));
+ if (isFirstExpansion(FID, ArgStart, SM)) {
+ SourceLocation ArgEnd =
----------------
when false is the fallthrough to handling as if part of the macro body
deliberate?
Thinking about it I suppose either that or returning NoTokens works, but please
document it, e.g. `} else { /* fall through and treat as part of the macro body
*/}`
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72041/new/
https://reviews.llvm.org/D72041
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
