This is an archive of the discontinued LLVM Phabricator instance.

[include-cleaner] Remove filtering from UsingDecl visit.
ClosedPublic

Authored by VitaNuo on Nov 28 2022, 7:29 AM.

Details

Summary

Removes filtering from the VisitUsingDecl method for implementation files.

Diff Detail

Event Timeline

VitaNuo created this revision.Nov 28 2022, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 7:29 AM
Herald added a subscriber: kadircet. · View Herald Transcript
VitaNuo requested review of this revision.Nov 28 2022, 7:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 7:29 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
VitaNuo updated this revision to Diff 478265.Nov 28 2022, 9:13 AM

Add another example.

VitaNuo retitled this revision from Remove filtering from UsingDecl visit. to [include-cleaner] Remove filtering from UsingDecl visit..Nov 28 2022, 9:14 AM
VitaNuo updated this revision to Diff 478267.Nov 28 2022, 9:15 AM

Edit commit message, add "[include-cleaner]".

hokein added inline comments.Nov 29 2022, 5:30 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
86

we should report all references as explicit.

87

The member field IsHeader is no longer used anywhere, we can remove it, which will simplify a few places, @kadircet any thoughts?

VitaNuo updated this revision to Diff 478556.Nov 29 2022, 6:17 AM

Address review comments: turn all references into explicit, remove the "isHeader" field.

VitaNuo marked an inline comment as done.Nov 29 2022, 6:19 AM
VitaNuo added inline comments.
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
87

Ok, removed. Let me know if I should undo that eventually.

hokein added inline comments.Nov 30 2022, 6:59 AM
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
130

nit: update the comment.

140–145

I think the c++-header is not needed, we can even remove the special c++-header logic in testWalk.

148

nit: the comment is stale, we report all refs as explicit now. I think we can just remove this case.

kadircet added inline comments.Nov 30 2022, 7:01 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
86

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?

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
136

you can drop this test now, having declaration in a header vs not doesn't make any difference.

149

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?

VitaNuo marked 4 inline comments as done.Dec 1 2022, 2:43 AM
VitaNuo added inline comments.
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
86

@hokein so what would be the final conclusion then? Should I re-introduce the "isUsed" check?

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
136

sure

140–145

Ok, removed the header logic from testWalk, too.

149

Yes, you're right. It probably only made sense before I removed the "isUsed" check.

VitaNuo updated this revision to Diff 479216.Dec 1 2022, 2:44 AM
VitaNuo marked 2 inline comments as done.

Address review comments, remove excessive tests.

hokein added inline comments.Dec 1 2022, 3:07 AM
clang-tools-extra/include-cleaner/lib/WalkAST.cpp
86

oh, right. Ambiguous is better, this is similar to OverloadExpr, we can't prove that the symbol is used. (sorry, I somewhat had an impression this should be explicit).

VitaNuo updated this revision to Diff 480375.Dec 6 2022, 1:06 AM

Re-introduce the isUsed check.

clang-tools-extra/include-cleaner/lib/WalkAST.cpp
86

Ok, thank you. I have re-introduced the isUsed check now.

VitaNuo marked 2 inline comments as done.Dec 6 2022, 1:07 AM
hokein added inline comments.Dec 6 2022, 2:50 AM
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
130

this comment is stale, and the below one as well.

VitaNuo updated this revision to Diff 480418.Dec 6 2022, 3:09 AM

Change/remove stale comments.

VitaNuo marked 2 inline comments as done.Dec 6 2022, 3:10 AM
VitaNuo added inline comments.
clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
130

oops, sorry.

149

I have re-introduced this test case now, since there's a differentiation between used and unused again.

hokein accepted this revision.Dec 8 2022, 1:53 AM

Thanks.

You can add a Fix: https://github.com/llvm/llvm-project/issues/59147 in the commit message, it will automatically close the github issue when you commit the patch.

clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp
152

nit: merge the following line with this line, })cpp"

160

nit: fix the alignment

161

nit: and here as well.

This revision is now accepted and ready to land.Dec 8 2022, 1:53 AM
VitaNuo updated this revision to Diff 481213.Dec 8 2022, 2:23 AM
VitaNuo marked 4 inline comments as done.

Formatting fixes.

This revision was landed with ongoing or failed builds.Dec 8 2022, 2:26 AM
This revision was automatically updated to reflect the committed changes.