[PATCH] D111698: [clangd] IncludeCleaner: Handle macros coming from ScratchBuffer

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:124
     const auto &Exp = SM.getSLocEntry(FID).getExpansion();
-    add(Exp.getSpellingLoc());
+    if (!SM.isWrittenInScratchSpace(Exp.getSpellingLoc()))
+      add(Exp.getSpellingLoc());
----------------
Add a comment


================
Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:126
+      add(Exp.getSpellingLoc());
     add(Exp.getExpansionLocStart());
     add(Exp.getExpansionLocEnd());
----------------
I'm not 100% sure these should be unconditional. It's *probably* right, but...
Can you add a test of the form:

```
#define ab x
#define concat(x, y) x##y
int foo(a, b);
```

and verify that we get the expected set of file IDs when seeding with the location of the VarDecl `x`?


================
Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:170
 
+TEST(IncludeCleaner, ScratchBuffer) {
+  TestTU TU;
----------------
this doesn't seem to test very much, a comment should indicate that this is guarding against a crash.

Ideally we'd (also?) have a test that calls findReferencedFiles and actually assert which IDs we get, rather than run some big blob of code and hope not to crash.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111698