[PATCH] D148793: [clang-tidy] Implement an include-cleaner check.

[Haojian Wu via Phabricator via cfe-commits]

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