[PATCH] D138821: [include-cleaner] Remove filtering from UsingDecl visit.

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:86
       report(UD->getLocation(), TD,
              IsUsed ? RefType::Explicit : RefType::Ambiguous);
     }
----------------
hokein wrote:
> we should report all references as explicit.
i think having `Ambiguous` here for unused symbols is fine. we'd like to consider such symbols for the purposes of saying "yeah this include is probably used" but we shouldn't be inserting headers for the unused ones.

do we have an example for the contrary?


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:136
            "using ns::^x; void foo() { x(); }");
-  // We should report unused overloads if main file is a header.
   testWalk(R"cpp(
     namespace ns {
----------------
you can drop this test now, having declaration in a header vs not doesn't make any difference.


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:149
+  // We should report references to templates as ambiguous.
+  testWalk(R"cpp(
+    namespace ns {
----------------
sorry if i am being dense but what's the difference we're trying to grasp between this and the next test case exactly?
i guess it's meant to test the difference between used and non-used template patterns?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138821