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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
