hokein added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:45
+ SourceLocation SymRefLocation;
+ std::string MissingHeaderSpelling;
+};
----------------
storing a string instance for header per system reference is expensive (we
might have many missing-include symbols, and we might have many duplications).
We can store the a `clang::include_cleaner::Header` in the struct here, and
call the `spellHeader` when generating the FixIts.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:88
+ SourceLocation Loc = D->getLocation();
+ if (!SM->isWrittenInMainFile(SM->getSpellingLoc(Loc)))
+ continue;
----------------
We should use the `getExpansionLoc` rather than the `SpellingLoc` here,
otherwise we might miss the case like `TEST_F(WalkUsedTest, MultipleProviders)
{... }` where the decl location is spelled in another file, but the function
body is spelled in main-file.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:89
+ if (!SM->isWrittenInMainFile(SM->getSpellingLoc(Loc)))
+ continue;
+ MainFileDecls.push_back(D);
----------------
and we probably need the same logic to filtering out implicit template
specialization, similar to
https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/include-cleaner/lib/Record.cpp#L408-L410.
It is fine to leave it in this patch, but please add a FIXME.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:130
+ // header mappings. But that's not different than rest of the places.
+ if (ClangTidyCheck::getCurrentMainFile().endswith(PHeader))
+ continue;
----------------
nit: the prefix `ClangTidyCheck::` qualifier is not needed, you can remove it.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:149
+ for (const auto *Inc : Unused) {
+ diag(Inc->HashLocation, "unused include %0")
+ << Inc->quote()
----------------
The diagnostic message "unused include ..." seems too short for users to
understand what's the issue here, it would be better to make it more
descriptive, in clangd, we emit `included header ... is not used directly`, we
can follow this pattern.
The same for the missing-includes.
================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:36
+ IncludeCleanerCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {
+ std::optional<llvm::StringRef> IgnoreHeaders =
----------------
nit: the function body is long, consider moving it to .cpp file.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:220
+ // include-cleaner is directly integrated in IncludeCleaner.cpp
+ "-misc-include-cleaner",
+
----------------
nit: the entry here is under the `Crashing Checks` category, this doesn't seem
a good fit. Maybe move it right after the above empty string `""`.
================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:13
+
+.. option:: IgnoreHeader
+
----------------
IgnoreHeader => IgnoreHeaders, in the implementation, we use the plural form.
================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:1
+#include "ClangTidyDiagnosticConsumer.h"
+#include "ClangTidyOptions.h"
----------------
nit: missing the license file comment.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D148793/new/
https://reviews.llvm.org/D148793
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
