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