[PATCH] D148793: [WIP][clang-tidy] Implement an include-cleaner check.
Kadir Cetinkaya via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu May 11 05:29:11 PDT 2023
kadircet added a comment.
thanks, mostly LG at the high level i think one thing we're missing is the ability to suppress analysis for certain headers, similar to what we have in clangd config options. can you add a check option for that?
================
Comment at: clang-tools-extra/clang-tidy/CMakeLists.txt:10
include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR})
+include_directories(BEFORE "${CMAKE_CURRENT_SOURCE_DIR}/../include-cleaner/include")
----------------
can you move this into `clang-tools-extra/clang-tidy/misc/CMakeLists.txt` ? in theory it isn't needed by all the checks
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:49
+void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
+ TUDecl = const_cast<Decl *>(Result.Nodes.getNodeAs<Decl>("top"));
+ SM = Result.SourceManager;
----------------
rather than storing state in `::check` and running logic in `::onEndOfTranslationUnit`, you can just trigger analysis & finding generation here as we already match only on the translation unit decl.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:57
+ std::vector<MissingIncludeInfo> Missing;
+ walkUsed(TUDecl, RecordedPreprocessor.MacroReferences, &RecordedPI, *SM,
+ [&](const include_cleaner::SymbolReference &Ref,
----------------
TUDecl can have children that are not part of the main file and we'll just run analysis on these decls for nothing as we discard references spelled outside mainfile inside `walkUsed`.
so can you go over the children of TUDecl, and only pick the ones that are spelled inside the mainfile and feed them as analysis roots?
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:93
+ // header mappings. But that's not different than rest of the places.
+ if (MainFile->tryGetRealPathName().endswith(PHeader))
+ continue;
----------------
you can use `ClangTidyCheck::getCurrentMainFile()` instead of `Mainfile->tryGetRealPathName()`
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:104
+
+ diag(Inc->HashLocation, Description) << FixItHint::CreateRemoval(
+ {Inc->HashLocation,
----------------
you can have: `diag(Inc->HashLocation, "unused include %0") << Inc->quote() << FixItHint::CreateRemoval(..)` instead of the string concat above.
(also convention is to have diagnostic messages that start with lower case letters)
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:106-109
+ SM->getComposedLoc(SM->getMainFileID(),
+ SM->getDecomposedLoc(Inc->HashLocation).second +
+ std::string("#include").length() +
+ Inc->quote().length() + 1)});
----------------
you can use `tooling::HeaderIncludes` not only for insertions, but also for removals.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:113
+ auto FileStyle = format::getStyle(
+ format::DefaultFormatStyle, MainFile->tryGetRealPathName(),
+ format::DefaultFallbackStyle, SM->getBufferData(SM->getMainFileID()),
----------------
again you can use `ClangTidyCheck::getCurrentMainFile()`
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:114
+ format::DefaultFormatStyle, MainFile->tryGetRealPathName(),
+ format::DefaultFallbackStyle, SM->getBufferData(SM->getMainFileID()),
+ &SM->getFileManager().getVirtualFileSystem());
----------------
nit: i'd extract `SM->getBufferData(SM->getMainFileID())` into a `llvm::StringRef Code` and use it in both places
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:116
+ &SM->getFileManager().getVirtualFileSystem());
+ if (!FileStyle) {
+ FileStyle = format::getLLVMStyle();
----------------
no need for braces
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:123-124
+
+ if (MainFile->getName().empty())
+ continue;
+ tooling::HeaderIncludes HeaderIncludes(
----------------
again you can use `ClangTidyCheck::getCurrentMainFile()` and drop the check
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:125
+ continue;
+ tooling::HeaderIncludes HeaderIncludes(
+ MainFile->getName(), SM->getBufferData(SM->getMainFileID()),
----------------
constructor of `tooling::HeaderIncludes` parses the code, so let's create it once outside the loop?
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:139
+
+ diag(Inc.SymRefLocation, Description) << FixItHint::CreateInsertion(
+ SM->getComposedLoc(SM->getMainFileID(), Replacement->getOffset()),
----------------
you can use the substitution alternative mentioned above here as well
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:25
+
+struct MissingIncludeInfo {
+ SourceLocation SymRefLocation;
----------------
can you move this struct into the implementation file? as it isn't exposed in the interfaces
================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:169
+
+ Checks for unused and missing includes.
+
----------------
`Enforces include-what-you-use on headers using include-cleaner.` ?
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:293
`modernize-use-bool-literals <modernize/use-bool-literals.html>`_, "Yes"
- `modernize-use-default-member-init <modernize/use-default-member-init.html>`_, "Yes"
+ `modernize-use-default-member-init <modernize/use-default-member-init.html>`_
`modernize-use-emplace <modernize/use-emplace.html>`_, "Yes"
----------------
could you revert this change?
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/list.rst:481
`cppcoreguidelines-non-private-member-variables-in-classes <cppcoreguidelines/non-private-member-variables-in-classes.html>`_, `misc-non-private-member-variables-in-classes <misc/non-private-member-variables-in-classes.html>`_,
- `cppcoreguidelines-use-default-member-init <cppcoreguidelines/use-default-member-init.html>`_, `modernize-use-default-member-init <modernize/use-default-member-init.html>`_,
+ `cppcoreguidelines-use-default-member-init <cppcoreguidelines/use-default-member-init.html>`_, `modernize-use-default-member-init <modernize/use-default-member-init.html>`_, "Yes"
`fuchsia-header-anon-namespaces <fuchsia/header-anon-namespaces.html>`_, `google-build-namespaces <google/build-namespaces.html>`_,
----------------
could you revert this change?
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:6
+
+Checks for unused and missing includes.
+Findings correspond to https://clangd.llvm.org/design/include-cleaner.
----------------
can you also mention that this only generates findings for the main file of a translation unit?
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:153
RecordPragma(const CompilerInstance &CI, PragmaIncludes *Out)
: SM(CI.getSourceManager()),
HeaderInfo(CI.getPreprocessor().getHeaderSearchInfo()), Out(Out),
----------------
can you change this constructor to ` : RecordPragma(CI.getSourceManager(), CI.getPreprocessor(), Out) `?
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:157
+ RecordPragma(const SourceManager &SM, const Preprocessor &P, PragmaIncludes *Out)
+ : SM(SM),
+ HeaderInfo(P.getHeaderSearchInfo()), Out(Out),
----------------
no need to pass `SM` seperately, you can get it out of pre-processor via `PP.getSourceManager()`
================
Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:351
+void PragmaIncludes::record(const SourceManager &SM, Preprocessor &P) {
+ auto Record = std::make_unique<RecordPragma>(SM, P, this);
----------------
same here, no need to pass `SM` separately
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148793/new/
https://reviews.llvm.org/D148793
More information about the cfe-commits
mailing list