nridge marked 4 inline comments as done.
nridge added inline comments.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:261
+ // consider it selected.
+ if (!SeenMacroCalls.insert(ArgStart).second) {
+ return NoTokens;
----------------
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`)?
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
