[clang-tools-extra] 89d0a76 - [clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol
Kadir Cetinkaya via cfe-commits
cfe-commits at lists.llvm.org
Tue Aug 8 07:02:13 PDT 2023
Author: Kadir Cetinkaya
Date: 2023-08-08T16:01:33+02:00
New Revision: 89d0a76be68b866779ac41e56bc7f7b4fc452f47
URL: https://github.com/llvm/llvm-project/commit/89d0a76be68b866779ac41e56bc7f7b4fc452f47
DIFF: https://github.com/llvm/llvm-project/commit/89d0a76be68b866779ac41e56bc7f7b4fc452f47.diff
LOG: [clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol
We received some user feedback around this being disruptful rather than
useful in certain workflows so add an option to control the output behaviour.
Differential Revision: https://reviews.llvm.org/D157390
Added:
Modified:
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
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
index c5cb18978b6497..f47609b19badfc 100644
--- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
+++ b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp
@@ -55,7 +55,9 @@ IncludeCleanerCheck::IncludeCleanerCheck(StringRef Name,
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 @@ IncludeCleanerCheck::IncludeCleanerCheck(StringRef Name,
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 @@ void IncludeCleanerCheck::check(const MatchFinder::MatchResult &Result) {
// 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
diff erent 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 &&
diff --git a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
index d5f75f2b1c7fa2..9641c7fd9dfcf6 100644
--- a/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
+++ b/clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.h
@@ -44,6 +44,8 @@ class IncludeCleanerCheck : public ClangTidyCheck {
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);
};
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 2af86407069a68..815237656a4035 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -192,6 +192,9 @@ Changes in existing checks
<clang-tidy/checks/readability/identifier-naming>` check to emit proper
warnings when a type forward declaration precedes its definition.
+- Misc-include-cleaner check has option `DeduplicateFindings` to output one
+ finding per occurence of a symbol.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst b/clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
index 3246fea78cd427..d56bb4526ff015 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/misc/include-cleaner.rst
@@ -33,3 +33,8 @@ Options
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.
diff --git a/clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp b/clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
index db048f10a0c152..fe3e38958f8985 100644
--- a/clang-tools-extra/unittests/clang-tidy/IncludeCleanerTest.cpp
+++ b/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 @@ int BazResult = baz();
)"}}));
}
+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"
More information about the cfe-commits
mailing list