This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG89d0a76be68b: [clang-tidy][include-cleaner] Add option to control deduplication of findingsā¦ (authored by kadircet).
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 @@ -192,6 +192,9 @@ <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 ^^^^^^^^^^^^^^ 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