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