[PATCH] D77507: [clangd] Fix HitMapping assertion in Tokens.cpp

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for tracking this down. Unfortunately I don't think removing the assertion is enough, the code doesn't handle this case correctly.



================
Comment at: clang/unittests/Tooling/Syntax/TokensTest.cpp:487
 )"},
+      // Baseline case, bugz: https://bugs.llvm.org/show_bug.cgi?id=45428
+      {R"cpp(
----------------
This is the case that always worked, right? Not sure we need to add this test.


================
Comment at: clang/unittests/Tooling/Syntax/TokensTest.cpp:505
+      // Causes mapping miss when invoking tryConsumeSpelledUntil
+      {R"cpp(
+    #define NUM 42
----------------
>From the linked bug, the varags isn't essential to this crash.
The linked example can be simplified a little further by removing foo:

```
#define N 1
#define ID(X) X
#define ID2 ID
ID2(N)
```

This makes it clearer that the trigger is using a macro in a arg to an "indirect" function macro (i.e. the function macro's name is expanded from another macro).


================
Comment at: clang/unittests/Tooling/Syntax/TokensTest.cpp:519
+    ['#'_0, 'void'_34) => ['void'_0, 'void'_0)
+    ['M0'_40, 'NUM'_44) => ['{'_6, ';'_10)
+    ['NUM'_44, ')'_45) => [';'_10, ';'_10)
----------------
The problem with just removing the assertion is that this behavior is wrong: it's mapping `M0(,` onto `{ 42 ; }` - i.e. the whole M0 macro expansion. Then all the other mappings are empty. Compare to the baseline case above...

This should be `[M0_42, ;_46)] => [{_6, ;_10]` I think.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77507