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

[Kadir Cetinkaya via Phabricator via cfe-commits]

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