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