Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp | ||
---|---|---|
2 | nit: missing the license file comment. |
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp | ||
---|---|---|
171–176 | tooling::HeaderIncludes already handles the include style well (and more). In general, we recommend to use tooling::headerIncludes -- it was built for generic toolings that need to perform #include manipulations, it has been heavily used in clangd/clang-format/clang-include-cleaner etc. IncludeInserter::createIncludeInsertion is an old implementation in clang-tidy, and it was built before tooling::HeaderIncludes is thing. In the long-term, I think we should probably consider deprecating it and replacing it with tooling::HeaderIncludes. |
Thanks for the comments!
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp | ||
---|---|---|
82 |
| |
87 | const will not work, since the container I need to populate has to have llvm::SmallVector<Decl *> type due to the include-cleaner library interfaces. | |
89 |
These are earlier comments from Kadir on this topic. AFAIU we want the spelling location for TEST_F(WalkUsedTest, MultipleProviders) {... } because:
| |
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h | ||
43 | Thanks for the tip, I can use the OptionsUtils infrastructure indeed. | |
clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp | ||
20 | they don't, adding a declaration to the custom "vector.h" and then using it in the main file makes vector disappear from the list of unused includes. |
The only thing is the source location, see my comments for details, otherwise looks good in general.
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp | ||
---|---|---|
89 | I think I'm not convinced, see my comments below.
I think it is true for some cases. For example, a function decl has different parts (declarator, and function body), if the declarator is introduced by a macro which defined in a header, and the function body is spelled in the main-file, we still want to analyze the function body, see a simple example below: // foo.h #define DECLARE(X) void X() // main.cpp #include "foo.h" DECLARE(myfunc) { int a; ... } Moreover, we have use ExpansionLoc pattern in other places when we collect the main-file top-level decl (include-cleaner, clangd), and I don't see a special reason to change the pattern in the clang-tidy check. | |
117 | it is sad that we duplicate the include-cleaner analyse implementation (the only difference is that here we record the references for missing-includes), I think we should find a way to share the code in the future. No action required in this patch, can you add a FIXME? | |
195 | I think we should use the spelling location -- Inc.SymRefLocation can be a macro location which can points to another file, we intend to show diagnostics for main-file only, and walkUsed has some internal logic which guarantees that the spelling loc of returned refs is main-file. | |
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h | ||
22 | can you cleanup the includes here? looks like there are some unused-includes now, at least Syntax/Tokens.h from the first glance. | |
clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst | ||
5 | nit: remove extra trailing == | |
clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp | ||
2 | nit: IncludeCleanerTest.cpp |
Thanks for the comments!
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp | ||
---|---|---|
89 | Is the expansion location of myfunc in the main file? If that's the case, we need the expansion location indeed. |
Thanks, just a few nits. I think it is in a good shape, let's ship it!
Please make sure the premerge-test is happy before landing the patch, the windows premerge test is failing now (https://buildkite.com/llvm-project/premerge-checks/builds/155660#0188764d-dc9f-4e8f-b489-c7b8f8b0c52a)
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp | ||
---|---|---|
89 |
Right. The expansion location here is a file location which points to the main-file. Would be nice if you add the above test into the unittest. | |
190–194 | nit: we can simplify it like if (auto Replacement = HeaderIncludes.insert(...)) diag(...); | |
clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp | ||
28 | wrap all things into an anonymous namespace to avoid potential ODR violations. | |
71 | Instead of using 4 += statements, I think it is a bit clearer to use llvm::formatv("bar.h;{1};{2};...", appendPathSystemIndependent(...), ....); |
I'm getting completely incomprehensible build errors on Gentoo from this, as of aa7eace8431ba213c5431638b894b0e1b4b481c7:
samu: job failed: : && /usr/lib/ccache/bin/x86_64-pc-linux-gnu-g++ -march=znver2 --param=l1-cache-size=32 --param=l1-cache-line-size=64 -O2 -pipe -frecord-gcc-switches -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -fno-lifetime-dse -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wimplicit-fallthrough -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -pedantic -Wno-long-long -Wl,-O1 -Wl,--as-needed -Wl,--hash-style=gnu -Wl,--export-dynamic -rdynamic -Wl,-rpath-link,/var/tmp/portage/sys-devel/clang-17.0.0_pre20230603/work/x/y/clang-abi_x86_64.amd64/./lib64 tools/extra/clang-tidy/tool/CMakeFiles/clang-tidy.dir/ClangTidyToolMain.cpp.o -o bin/clang-tidy -L/usr/lib/llvm/17/lib64 -Wl,-rpath,"\$ORIGIN/../lib64:/usr/lib/llvm/17/lib64:/var/tmp/portage/sys-devel/clang-17.0.0_pre20230603/work/x/y/clang-abi_x86_64.amd64/lib64:" lib64/libclangTidy.a lib64/libclangTidyMain.a lib64/libclangTidyAndroidModule.a lib64/libclangTidyAbseilModule.a lib64/libclangTidyAlteraModule.a lib64/libclangTidyBoostModule.a lib64/libclangTidyBugproneModule.a lib64/libclangTidyCERTModule.a lib64/libclangTidyConcurrencyModule.a lib64/libclangTidyCppCoreGuidelinesModule.a lib64/libclangTidyDarwinModule.a lib64/libclangTidyFuchsiaModule.a lib64/libclangTidyGoogleModule.a lib64/libclangTidyHICPPModule.a lib64/libclangTidyLinuxKernelModule.a lib64/libclangTidyLLVMModule.a lib64/libclangTidyLLVMLibcModule.a lib64/libclangTidyMiscModule.a lib64/libclangTidyModernizeModule.a lib64/libclangTidyObjCModule.a lib64/libclangTidyOpenMPModule.a lib64/libclangTidyPerformanceModule.a lib64/libclangTidyPortabilityModule.a lib64/libclangTidyReadabilityModule.a lib64/libclangTidyZirconModule.a lib64/libclangTidyMPIModule.a lib64/libclangTidyBugproneModule.a lib64/libclangTidyCppCoreGuidelinesModule.a lib64/libclangTidyGoogleModule.a lib64/libclangTidyMiscModule.a lib64/libclangAnalysis.a lib64/libclangASTMatchers.a lib64/libclangAST.a lib64/libclangLex.a lib64/libclangBasic.a lib64/libclangTidyModernizeModule.a lib64/libclangTidyReadabilityModule.a lib64/libclangTidyUtils.a lib64/libclangTidy.a lib64/libclang-cpp.so.17gitaa7eace8 /usr/lib/llvm/17/lib64/libLLVM-17gitaa7eace8.so && : /usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: lib64/libclangTidyMiscModule.a(IncludeCleanerCheck.cpp.o): in function `clang::tidy::misc::IncludeCleanerCheck::registerPPCallbacks(clang::SourceManager const&, clang::Preprocessor*, clang::Preprocessor*) [clone .localalias]': IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck19registerPPCallbacksERKNS_13SourceManagerEPNS_12PreprocessorES7_+0x24): undefined reference to `clang::include_cleaner::RecordedPP::record(clang::Preprocessor const&)' /usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck19registerPPCallbacksERKNS_13SourceManagerEPNS_12PreprocessorES7_+0x94): undefined reference to `clang::include_cleaner::PragmaIncludes::record(clang::Preprocessor&)' /usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: lib64/libclangTidyMiscModule.a(IncludeCleanerCheck.cpp.o): in function `clang::tidy::misc::IncludeCleanerCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&) [clone .localalias]': IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0x508): undefined reference to `clang::include_cleaner::walkUsed(llvm::ArrayRef<clang::Decl*>, llvm::ArrayRef<clang::include_cleaner::SymbolReference>, clang::include_cleaner::PragmaIncludes const*, clang::SourceManager const&, llvm::function_ref<void (clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)>)' /usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0x604): undefined reference to `clang::include_cleaner::PragmaIncludes::getPublic(clang::FileEntry const*) const' /usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0x9e8): undefined reference to `clang::include_cleaner::Include::quote[abi:cxx11]() const' /usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: IncludeCleanerCheck.cpp:(.text._ZN5clang4tidy4misc19IncludeCleanerCheck5checkERKNS_12ast_matchers11MatchFinder11MatchResultE+0xd15): undefined reference to `clang::include_cleaner::spellHeader[abi:cxx11](clang::include_cleaner::Header const&, clang::HeaderSearch const&, clang::FileEntry const*)' /usr/lib/gcc/x86_64-pc-linux-gnu/13/../../../../x86_64-pc-linux-gnu/bin/ld: lib64/libclangTidyMiscModule.a(IncludeCleanerCheck.cpp.o): in function `void llvm::function_ref<void (clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)>::callback_fn<clang::tidy::misc::IncludeCleanerCheck::check(clang::ast_matchers::MatchFinder::MatchResult const&)::{lambda(clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)#1}>(long, clang::include_cleaner::SymbolReference const&, llvm::ArrayRef<clang::include_cleaner::Header>)': IncludeCleanerCheck.cpp:(.text._ZN4llvm12function_refIFvRKN5clang15include_cleaner15SymbolReferenceENS_8ArrayRefINS2_6HeaderEEEEE11callback_fnIZNS1_4tidy4misc19IncludeCleanerCheck5checkERKNS1_12ast_matchers11MatchFinder11MatchResultEEUlS5_S8_E_EEvlS5_S8_+0xd1): undefined reference to `clang::include_cleaner::Includes::match(clang::include_cleaner::Header) const' collect2: error: ld returned 1 exit status
I'm also hitting this; it only seems to happen if building with -DLLVM_LINK_LLVM_DYLIB=ON.
My educated guess would be that clangIncludeCleaner is being linked via clang_target_link_libraries while it's not part of libclang-cpp, so it should go into regular target_link_libraries.
Yes, that does seem to do the trick. LGTM from my PoV if you can push such a fix, otherwise I can try to do it in a little while...
Looks like it is fixed in https://github.com/llvm/llvm-project/commit/ac0ea7555ee4ae872bcd153e04513ba0b88b8985, thanks!