kadircet updated this revision to Diff 548190.
kadircet marked 2 inline comments as done.
kadircet added a comment.
Address comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157390/new/
https://reviews.llvm.org/D157390
Files:
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
Index: clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
===================================================================
--- clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
+++ clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
@@ -15,6 +15,8 @@
#include "llvm/Support/FormatVariadic.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/Regex.h"
+#include "llvm/Testing/Annotations/Annotations.h"
+#include "gmock/gmock.h"
#include "gtest/gtest.h"
#include <initializer_list>
@@ -108,6 +110,50 @@
)"}}));
}
+TEST(IncludeCleanerCheckTest, DedupsMissingIncludes) {
+ llvm::Annotations Code(R"(
+#include "baz.h" // IWYU pragma: keep
+
+int BarResult1 = $diag1^bar();
+int BarResult2 = $diag2^bar();)");
+
+ {
+ std::vector<ClangTidyError> Errors;
+ runCheckOnCode<IncludeCleanerCheck>(Code.code(), &Errors, "file.cpp",
+ std::nullopt, ClangTidyOptions(),
+ {{"baz.h", R"(#pragma once
+ #include "bar.h"
+ )"},
+ {"bar.h", R"(#pragma once
+ int bar();
+ )"}});
+ ASSERT_THAT(Errors.size(), testing::Eq(1U));
+ EXPECT_EQ(Errors.front().Message.Message,
+ "no header providing \"bar\" is directly included");
+ EXPECT_EQ(Errors.front().Message.FileOffset, Code.point("diag1"));
+ }
+ {
+ std::vector<ClangTidyError> Errors;
+ ClangTidyOptions Opts;
+ Opts.CheckOptions.insert({"DeduplicateFindings", "false"});
+ runCheckOnCode<IncludeCleanerCheck>(Code.code(), &Errors, "file.cpp",
+ std::nullopt, Opts,
+ {{"baz.h", R"(#pragma once
+ #include "bar.h"
+ )"},
+ {"bar.h", R"(#pragma once
+ int bar();
+ )"}});
+ ASSERT_THAT(Errors.size(), testing::Eq(2U));
+ EXPECT_EQ(Errors.front().Message.Message,
+ "no header providing \"bar\" is directly included");
+ EXPECT_EQ(Errors.front().Message.FileOffset, Code.point("diag1"));
+ EXPECT_EQ(Errors.back().Message.Message,
+ "no header providing \"bar\" is directly included");
+ EXPECT_EQ(Errors.back().Message.FileOffset, Code.point("diag2"));
+ }
+}
+
TEST(IncludeCleanerCheckTest, SuppressMissingIncludes) {
const char *PreCode = R"(
#include "bar.h"
Index: clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
+++ clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
@@ -33,3 +33,8 @@
files that match this regex as a suffix. E.g., `foo/.*` disables
insertion/removal for all headers under the directory `foo`. By default, no
headers will be ignored.
+
+.. option:: DeduplicateFindings
+
+ A boolean that controls whether the check should deduplicate findings for the
+ same symbol. Defaults to true.
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -163,6 +163,9 @@
<clang-tidy/checks/bugprone/reserved-identifier>`, so that it does not warn
on macros starting with underscore and lowercase letter.
+- Misc-include-cleaner check has option `DeduplicateFindings` to output one
+ finding per occurence of a symbol.
+
Removed checks
^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
===================================================================
--- clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
+++ clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
@@ -44,6 +44,8 @@
include_cleaner::PragmaIncludes RecordedPI;
HeaderSearch *HS;
std::vector<StringRef> IgnoreHeaders;
+ // Whether emit only one finding per usage of a symbol.
+ const bool DeduplicateFindings;
llvm::SmallVector<llvm::Regex> IgnoreHeadersRegex;
bool shouldIgnore(const include_cleaner::Header &H);
};
Index: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
+++ clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
@@ -55,7 +55,9 @@
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IgnoreHeaders(utils::options::parseStringList(
- Options.getLocalOrGlobal("IgnoreHeaders", ""))) {
+ Options.getLocalOrGlobal("IgnoreHeaders", ""))),
+ DeduplicateFindings(
+ Options.getLocalOrGlobal("DeduplicateFindings", true)) {
for (const auto &Header : IgnoreHeaders) {
if (!llvm::Regex{Header}.isValid())
configurationDiag("Invalid ignore headers regex '%0'") << Header;
@@ -69,6 +71,7 @@
void IncludeCleanerCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IgnoreHeaders",
utils::options::serializeStringList(IgnoreHeaders));
+ Options.store(Opts, "DeduplicateFindings", DeduplicateFindings);
}
bool IncludeCleanerCheck::isLanguageVersionSupported(
@@ -114,12 +117,26 @@
// FIXME: Filter out implicit template specializations.
MainFileDecls.push_back(D);
}
+ llvm::DenseSet<include_cleaner::Symbol> SeenSymbols;
// FIXME: Find a way to have less code duplication between include-cleaner
// analysis implementation and the below code.
walkUsed(MainFileDecls, RecordedPreprocessor.MacroReferences, &RecordedPI,
*SM,
[&](const include_cleaner::SymbolReference &Ref,
llvm::ArrayRef<include_cleaner::Header> Providers) {
+ // Process each symbol once to reduce noise in the findings.
+ // Tidy checks are used in two different workflows:
+ // - Ones that show all the findings for a given file. For such
+ // workflows there is not much point in showing all the occurences,
+ // as one is enough to indicate the issue.
+ // - Ones that show only the findings on changed pieces. For such
+ // workflows it's useful to show findings on every reference of a
+ // symbol as otherwise tools might give incosistent results
+ // depending on the parts of the file being edited. But it should
+ // still help surface findings for "new violations" (i.e.
+ // dependency did not exist in the code at all before).
+ if (DeduplicateFindings && !SeenSymbols.insert(Ref.Target).second)
+ return;
bool Satisfied = false;
for (const include_cleaner::Header &H : Providers) {
if (H.kind() == include_cleaner::Header::Physical &&
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
