[PATCH] D61681: [clangd] A code tweak to expand a macro

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clang-tools-extra/clangd/refactor/tweaks/ExpandMacro.cpp:57
+  if (It == Spelled.begin())
+    return Spelled.end();
+  // Check the token we found actually touches the cursor position.
----------------
sammccall wrote:
> it's pretty weird for a function whose return type is spelled "Token*" to use Spelled.end() rather than nullptr as a sentinel
Yeah, especially given that the function below uses `nullptr` as a sentinel...
Thanks for pointing this out!


================
Comment at: clang-tools-extra/clangd/unittests/TweakTests.cpp:288
+#define FUNC(X) X+X+X
+^F^O^O^ BAR ^F^O^O^
+^F^U^N^C^(1)
----------------
sammccall wrote:
> Can you verify we don't trigger here? `FOO[[ ]]BAR`
> 
> The zero-width range in `FOO^ BAR` is indeed interpreted as pointing at FOO by SelectionTree, but that's a whitespace-sensitive heuristic.
> 
> Given
> ```
> int x(int);
> #define B x
> int y = B^(42);
> ```
> 
> The `^` points at the `(`. (Maybe we should lift this logic into Tweak::Selection)
It does trigger, unfortunately. There is no way to unbreak this, as we don't have a way to get the selection range in inputs of the tweak.
I've added a FIXME, will address with a follow-up.

Note that we don't use selection tree, as it's AST-based and we need token-level information for that tweak (that's pretty unique, I expect most tweaks are not like that)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61681