[PATCH] D138779: [include-cleaner] Filter out references that not spelled in the main file.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:58
+bool isSpelledInMainFile(SourceLocation Loc, const SourceManager &SM) {
+  return SM.isWrittenInMainFile(Loc.isMacroID() ? SM.getSpellingLoc(Loc) : Loc);
+}
----------------
this seems like unneccesary indirection:
 - the argument is just SM.getSpellingLoc(Loc), no isMacroID check needed
 - and given this, maybe just inline?


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:148
+}
+TEST(WalkUsed, FilterRefsNotSpelledInMainFile) {
+  llvm::Annotations Main(R"cpp(
----------------
I find this test very hard to follow because it combines a lot of complicated and separate test cases, fixtures etc.
Consider a table-based test.


================
Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:218
+        switch (Ref.Target.kind()) {
+        case Symbol::Declaration:
+          SymbolRefs[dyn_cast<NamedDecl>(&Ref.Target.declaration())->getName()]
----------------
all this stuff around name-based lookup can be much simpler if you make your tests narrower, and require the ref target to have a fixed name (use `llvm::to_string(Ref.Target)`)

```
int ^target = 1;
#define M target
#define USE_M M
int y = ^USE_M;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138779