[PATCH] D72041: [clangd] Handle go-to-definition in macro invocations where the target appears in the expansion multiple times

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for fixing this. I think it could maybe use a further tweak, LMK if I'm missing something as my intuition around macros is pretty bad.



================
Comment at: clang-tools-extra/clangd/Selection.cpp:261
+      // consider it selected.
+      if (!SeenMacroCalls.insert(ArgStart).second) {
+        return NoTokens;
----------------
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.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:261
+      // consider it selected.
+      if (!SeenMacroCalls.insert(ArgStart).second) {
+        return NoTokens;
----------------
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).


================
Comment at: clang-tools-extra/clangd/Selection.h:58
 // SelectionTree tries to behave sensibly in the presence of macros, but does
 // not model any preprocessor concepts: the output is a subset of the AST.
 //
----------------
Worth mentioning the new behavior here, IMO. e.g.

```
When a macro argument is specifically selected, only its first expansion is selected in the AST.
(Returning a selection forest is unreasonably difficult for callers to handle correctly).
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72041