This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by hokein on Nov 28 2022, 1:35 AM.

Diff Detail

Event Timeline

hokein created this revision.Nov 28 2022, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 1:35 AM
hokein requested review of this revision.Nov 28 2022, 1:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 28 2022, 1:35 AM
sammccall added inline comments.Nov 28 2022, 1:55 AM
clang-tools-extra/include-cleaner/lib/FindHeaders.cpp
58 ↗(On Diff #478156)

this seems like unneccesary indirection:

  • the argument is just SM.getSpellingLoc(Loc), no isMacroID check needed
  • and given this, maybe just inline?
clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
149

I find this test very hard to follow because it combines a lot of complicated and separate test cases, fixtures etc.
Consider a table-based test.

219

all this stuff around name-based lookup can be much simpler if you make your tests narrower, and require the ref target to have a fixed name (use llvm::to_string(Ref.Target))

int ^target = 1;
#define M target
#define USE_M M
int y = ^USE_M;
kadircet added a comment.EditedNov 28 2022, 5:26 AM
This comment has been deleted.
clang-tools-extra/include-cleaner/lib/Analysis.cpp
20

as discussed offline we're introducing this extra constraint on references being provided, so let's mention that in the documentation as well. apart from that I agree with rest of the comments from Sam

hokein updated this revision to Diff 478228.Nov 28 2022, 6:55 AM
hokein marked 3 inline comments as done.

address comments:

  • polish and simplify the tests
  • inline the spelling-loc-check logic
  • add a comment for the walkUsed

this patch is ready for another review.

kadircet added inline comments.Dec 6 2022, 1:06 AM
clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h
36

maybe rather say Only reports references from main file ?

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
161

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).

177

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;)

239

nit: no need for optional SourceLocation will be invalid by default.

hokein updated this revision to Diff 481591.Dec 9 2022, 3:16 AM
hokein marked 4 inline comments as done.

address review comments

kadircet accepted this revision.Dec 9 2022, 3:22 AM

thanks!

clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp
297

nit: this and the following test case is pretty much the same. maybe drop this one?

This revision is now accepted and ready to land.Dec 9 2022, 3:22 AM
This revision was landed with ongoing or failed builds.Dec 9 2022, 3:59 AM
This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.