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

[Viktoriia Bakalova via Phabricator via cfe-commits]

VitaNuo marked 4 inline comments as done.
VitaNuo added inline comments.


================
Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:86
       report(UD->getLocation(), TD,
              IsUsed ? RefType::Explicit : RefType::Ambiguous);
     }
----------------
kadircet wrote:
> 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?
@hokein so what would be the final conclusion then? Should I re-introduce the "isUsed" check?


================
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 {
----------------
kadircet wrote:
> you can drop this test now, having declaration in a header vs not doesn't make any difference.
sure


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:140
     })cpp",
            "// c++-header\n using ns::^x;");
+  testWalk(R"cpp(
----------------
hokein wrote:
> I think the `c++-header` is not needed, we can even remove the special `c++-header` logic in `testWalk`.
Ok, removed the header logic from testWalk, too.


================
Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:149
+  // We should report references to templates as ambiguous.
+  testWalk(R"cpp(
+    namespace ns {
----------------
kadircet wrote:
> 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?
Yes, you're right. It probably only made sense before I removed the "isUsed" check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138821