kadircet added inline comments.
================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:36 /// Find and report all references to symbols in a region of code. +/// It will filter out references that are not spelled in the main file. /// ---------------- maybe rather say `Only reports references from main file` ? ================ Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:161 + R"cpp( + int target(); + #define CALL_FUNC $spell^target() ---------------- i think it's better to make declarations for `target` always part of the header. otherwise there are changes to both `target` and extra step in between (e.g. CALL_FUNC) between tests (so multiple things could go wrong and cancel each other). ================ Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:177 + int target; + #define PLUS_ONE(X) X + 1 + int a = $expand^PLUS_ONE($spell^target); ---------------- nit: i think you can still have `int target()` and convert `PLUS_ONE` to `X() + 1` (same for other examples). (or you can do it the other way around, always have an `static int target = 0;`) ================ Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:239 + + std::optional<SourceLocation> RefLoc; + walkUsed(TopLevelDecls, Recorded.MacroReferences, ---------------- nit: no need for `optional` SourceLocation will be invalid by default. 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
