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