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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

Not too bad, you on a right road. Tests failed on windows, and clang-format failed.



================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:86
+  llvm::SmallVector<Decl *> MainFileDecls;
+  for (auto *D : Result.Nodes.getNodeAs<TranslationUnitDecl>("top")->decls()) {
+    SourceLocation Loc = D->getLocation();
----------------
auto -> const Decl* 


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp:170-175
+    diag(Inc.SymRefLocation, "missing include %0")
+        << Inc.MissingHeaderSpelling
+        << FixItHint::CreateInsertion(
+               SM->getComposedLoc(SM->getMainFileID(),
+                                  Replacement->getOffset()),
+               Replacement->getReplacementText());
----------------
Use IncludeInserter::createIncludeInsertion if possible...
So include style would be properly handled.


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:28-29
+
+/// Compute unused and missing includes and suggest fixes.
+/// Findings correspond to https://clangd.llvm.org/design/include-cleaner.
+///
----------------
this description does not match first sentence in check documentation.


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:42
+    llvm::SmallVector<llvm::StringRef> SplitIgnoreHeaders;
+    llvm::StringRef{*IgnoreHeaders}.split(SplitIgnoreHeaders, ",");
+    for (const auto &Header : SplitIgnoreHeaders) {
----------------
many check uses ; as separator, and uses parseStringList function to do that from OptionsUtils.


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:44
+    for (const auto &Header : SplitIgnoreHeaders) {
+      std::string HeaderSuffix{Header.str() + "$"};
+      if (!llvm::Regex{HeaderSuffix}.isValid())
----------------
if someone will put empty string like this `,,` you will end up with `$` as regex, wont it match "everything" ?


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:47
+        configurationDiag("Invalid ignore headers regex '%0'") << Header;
+      IgnoreHeadersRegex.push_back(llvm::Regex{HeaderSuffix});
+    }
----------------
llvm::Regex got constructor, use emplace_back


================
Comment at: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h:55
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override;
+
+private:
----------------
Missing "void storeOptions(ClangTidyOptions::OptionMap &Opts) override;"
to store IgnoreHeaders option. --export-config may not work correctly without this.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:189
+
+  Enforces include-what-you-use on headers using include-cleaner.
+
----------------
is this actually a "include-what-you-use" ?
Simply if I use iwyu tool, will it give same output and result in same behaviour ?
If this is different tool, then change this to avoid confusion.




================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:189
+
+  Enforces include-what-you-use on headers using include-cleaner.
+
----------------
PiotrZSL wrote:
> is this actually a "include-what-you-use" ?
> Simply if I use iwyu tool, will it give same output and result in same behaviour ?
> If this is different tool, then change this to avoid confusion.
> 
> 
This description does not match first sentence in check documentation.


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:6
+
+Checks for unused and missing includes. The check only generates findings for
+the main file of a translation unit.
----------------
avoid "The check"


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:9
+Findings correspond to https://clangd.llvm.org/design/include-cleaner.
+
+Options
----------------
Try adding some very simple "example"


================
Comment at: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst:17
+   files that match this regex as a suffix.  E.g., `foo/.*` disables
+   insertion/removal for all headers under the directory `foo`.
----------------
add some info about default value, is there any ?


================
Comment at: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp:9
+#include <vector>
+
+using namespace clang::tidy::misc;
----------------
this test file is fine, but there is no validation of output warning.



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D148793/new/

https://reviews.llvm.org/D148793