This is an archive of the discontinued LLVM Phabricator instance.

[include-cleaner][clangd][clang-tidy] Ignore resource dir during include-cleaner analysis.
ClosedPublic

Authored by VitaNuo on Aug 10 2023, 6:11 AM.

Diff Detail

Event Timeline

VitaNuo created this revision.Aug 10 2023, 6:11 AM
VitaNuo requested review of this revision.Aug 10 2023, 6:11 AM
kadircet added inline comments.Aug 10 2023, 8:34 AM
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
123

let's use HS->getModuleMap().getBuiltinDir() then we can get away with just comparing that pointer to H.physical()->getLastRef().getDir() (same applies to all the other places as well)

clang-tools-extra/clangd/IncludeCleaner.cpp
314

let's move this into mayConsiderUnused, we also convert this include into a FileEntry in there, so we can directly compare the directory agian.

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
579–580

can you rather include these as <amintrin.h>

587–588

we should put these under resources/include/xxxx.h

VitaNuo updated this revision to Diff 549443.Aug 11 2023, 9:43 AM
VitaNuo marked 4 inline comments as done.

Address review comments.

Thanks for the comments!

clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
123

This only works in clangd code for me. I get nullptr for HS->getModuleMap().getBuiltinDir() in other places (Clang Tidy check and the library).

clang-tools-extra/clangd/IncludeCleaner.cpp
314

Ok moved to mayConsiderUnused and using the file entry now, but see above regarding comparing the directories.

kadircet added inline comments.Aug 14 2023, 10:13 AM
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
123

hmm that's probably because you're not setting up the include structure correctly. resource-dir isn't the builtindir, it's resource-dir + "/include" (the comments i had in the clangd unittests applies in a similar manner to other tests as well).

VitaNuo updated this revision to Diff 555985.Sep 6 2023, 1:43 AM

Fix analysis test to properly recognize the resource directory.
Use HeaderSearch->getModuleMap().BuiltinDir to find out the resource directory.

VitaNuo marked an inline comment as done.Sep 6 2023, 1:44 AM

Thanks for the help in resolving the resource dir issues!

VitaNuo updated this revision to Diff 555995.Sep 6 2023, 2:49 AM

Rebase to current main.

kadircet accepted this revision.Sep 7 2023, 4:09 AM

thanks!

clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp
587

nit: also wrap contents in guard() here to make sure we're not testing multiple things here (i.e. behavior for non-self-contained headers)

clang-tools-extra/include-cleaner/lib/Analysis.cpp
100

now we're not going to put matching includes into the Used set. i know we're going to filter them put from the final output in the next loop, but still better to keep normal track of Used i believe

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

again lets make this self-contained

This revision is now accepted and ready to land.Sep 7 2023, 4:09 AM
VitaNuo updated this revision to Diff 556127.Sep 7 2023, 4:35 AM
VitaNuo marked 3 inline comments as done.

Address review comments.

This revision was landed with ongoing or failed builds.Sep 7 2023, 4:39 AM
This revision was automatically updated to reflect the committed changes.