[clang-tools-extra] af4f2eb - [clang-tidy] remove duplicate fixes of alias checkers
Nathan James via cfe-commits
cfe-commits at lists.llvm.org
Fri Jun 19 12:41:37 PDT 2020
Author: Daniel
Date: 2020-06-19T20:40:59+01:00
New Revision: af4f2eb476361e6da42d6f66a68cada763625c32
URL: https://github.com/llvm/llvm-project/commit/af4f2eb476361e6da42d6f66a68cada763625c32
DIFF: https://github.com/llvm/llvm-project/commit/af4f2eb476361e6da42d6f66a68cada763625c32.diff
LOG: [clang-tidy] remove duplicate fixes of alias checkers
when both a check and its alias are enabled, we should only take the fixes of one of them and not both.
This patch fixes bug 45577
https://bugs.llvm.org/show_bug.cgi?id=45577
Reviewed By: aaron.ballman, njames93
Differential Revision: https://reviews.llvm.org/D80753
Added:
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
Modified:
clang-tools-extra/clang-tidy/ClangTidy.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
llvm/include/llvm/ADT/StringMap.h
llvm/unittests/ADT/StringMapTest.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index d9b4b01e7161..163aad2ec378 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -122,6 +122,8 @@ class ErrorReporter {
{
auto Level = static_cast<DiagnosticsEngine::Level>(Error.DiagLevel);
std::string Name = Error.DiagnosticName;
+ if (!Error.EnabledDiagnosticAliases.empty())
+ Name += "," + llvm::join(Error.EnabledDiagnosticAliases, ",");
if (Error.IsWarningAsError) {
Name += ",-warnings-as-errors";
Level = DiagnosticsEngine::Error;
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
index 9742cd08d9c3..5c23323b03f0 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -27,6 +27,7 @@
#include "llvm/ADT/STLExtras.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/Support/Regex.h"
+#include "llvm/Support/FormatVariadic.h"
#include <tuple>
#include <vector>
using namespace clang;
@@ -634,6 +635,8 @@ void ClangTidyDiagnosticConsumer::removeIncompatibleErrors() {
std::tuple<unsigned, EventType, int, int, unsigned> Priority;
};
+ removeDuplicatedDiagnosticsOfAliasCheckers();
+
// Compute error sizes.
std::vector<int> Sizes;
std::vector<
@@ -728,3 +731,59 @@ std::vector<ClangTidyError> ClangTidyDiagnosticConsumer::take() {
removeIncompatibleErrors();
return std::move(Errors);
}
+
+namespace {
+struct LessClangTidyErrorWithoutDiagnosticName {
+ bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS) const {
+ const tooling::DiagnosticMessage &M1 = LHS->Message;
+ const tooling::DiagnosticMessage &M2 = RHS->Message;
+
+ return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
+ std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+ }
+};
+} // end anonymous namespace
+
+void ClangTidyDiagnosticConsumer::removeDuplicatedDiagnosticsOfAliasCheckers() {
+ using UniqueErrorSet =
+ std::set<ClangTidyError *, LessClangTidyErrorWithoutDiagnosticName>;
+ UniqueErrorSet UniqueErrors;
+
+ auto IT = Errors.begin();
+ while (IT != Errors.end()) {
+ ClangTidyError &Error = *IT;
+ std::pair<UniqueErrorSet::iterator, bool> Inserted =
+ UniqueErrors.insert(&Error);
+
+ // Unique error, we keep it and move along.
+ if (Inserted.second) {
+ ++IT;
+ } else {
+ ClangTidyError &ExistingError = **Inserted.first;
+ const llvm::StringMap<tooling::Replacements> &CandidateFix =
+ Error.Message.Fix;
+ const llvm::StringMap<tooling::Replacements> &ExistingFix =
+ (*Inserted.first)->Message.Fix;
+
+ if (CandidateFix != ExistingFix) {
+
+ // In case of a conflict, don't suggest any fix-it.
+ ExistingError.Message.Fix.clear();
+ ExistingError.Notes.emplace_back(
+ llvm::formatv("cannot apply fix-it because an alias checker has "
+ "suggested a
diff erent fix-it; please remove one of "
+ "the checkers ('{0}', '{1}') or "
+ "ensure they are both configured the same",
+ ExistingError.DiagnosticName, Error.DiagnosticName)
+ .str());
+ }
+
+ if (Error.IsWarningAsError)
+ ExistingError.IsWarningAsError = true;
+
+ // Since it is the same error, we should take it as alias and remove it.
+ ExistingError.EnabledDiagnosticAliases.emplace_back(Error.DiagnosticName);
+ IT = Errors.erase(IT);
+ }
+ }
+}
diff --git a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
index 2bb3296b150d..90cc4da3021f 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -48,6 +48,7 @@ struct ClangTidyError : tooling::Diagnostic {
bool IsWarningAsError);
bool IsWarningAsError;
+ std::vector<std::string> EnabledDiagnosticAliases;
};
/// Contains displayed and ignored diagnostic counters for a ClangTidy
@@ -246,6 +247,7 @@ class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
private:
void finalizeLastError();
void removeIncompatibleErrors();
+ void removeDuplicatedDiagnosticsOfAliasCheckers();
/// Returns the \c HeaderFilter constructed for the options set in the
/// context.
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp
new file mode 100644
index 000000000000..e6b6a1f23a23
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp
@@ -0,0 +1,23 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t -- \
+//// RUN: -config='{CheckOptions: [ \
+//// RUN: {key: cppcoreguidelines-pro-type-member-init.UseAssignment, value: 1}, \
+//// RUN: ]}'
+
+class Foo {
+public:
+ Foo() : _num1(0)
+ // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init,hicpp-member-init]
+ // CHECK-MESSAGES: note: cannot apply fix-it because an alias checker has suggested a
diff erent fix-it; please remove one of the checkers ('cppcoreguidelines-pro-type-member-init', 'hicpp-member-init') or ensure they are both configured the same
+ {
+ _num1 = 10;
+ }
+
+ int use_the_members() const {
+ return _num1 + _num2;
+ }
+
+private:
+ int _num1;
+ int _num2;
+ // CHECK-FIXES: _num2;
+};
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
new file mode 100644
index 000000000000..f67c20635064
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
@@ -0,0 +1,39 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+
+template <typename T>
+class vector {
+public:
+ void push_back(const T &) {}
+ void push_back(T &&) {}
+
+ template <typename... Args>
+ void emplace_back(Args &&... args){};
+};
+} // namespace std
+
+class Foo {
+public:
+ Foo() : _num1(0)
+ // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init,hicpp-member-init]
+ {
+ _num1 = 10;
+ }
+
+ int use_the_members() const {
+ return _num1 + _num2;
+ }
+
+private:
+ int _num1;
+ int _num2;
+ // CHECK-FIXES: _num2{};
+};
+
+int should_use_emplace(std::vector<Foo> &v) {
+ v.push_back(Foo());
+ // CHECK-FIXES: v.emplace_back();
+ // CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace,modernize-use-emplace]
+}
+
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
index 393c91566de2..c9d48d68a4c6 100644
--- a/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
@@ -2,8 +2,7 @@
void alwaysThrows() {
int ex = 42;
- // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err09-cpp]
- // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err61-cpp]
+ // CHECK-MESSAGES: warning: throw expression should throw anonymous temporary values instead [cert-err09-cpp,cert-err61-cpp]
throw ex;
}
diff --git a/llvm/include/llvm/ADT/StringMap.h b/llvm/include/llvm/ADT/StringMap.h
index fd8643fab150..840f328db796 100644
--- a/llvm/include/llvm/ADT/StringMap.h
+++ b/llvm/include/llvm/ADT/StringMap.h
@@ -248,6 +248,26 @@ class StringMap : public StringMapImpl {
return count(MapEntry.getKey());
}
+ /// equal - check whether both of the containers are equal.
+ bool operator==(const StringMap &RHS) const {
+ if (size() != RHS.size())
+ return false;
+
+ for (const auto &KeyValue : *this) {
+ auto FindInRHS = RHS.find(KeyValue.getKey());
+
+ if (FindInRHS == RHS.end())
+ return false;
+
+ if (!(KeyValue.getValue() == FindInRHS->getValue()))
+ return false;
+ }
+
+ return true;
+ }
+
+ bool operator!=(const StringMap &RHS) const { return !(*this == RHS); }
+
/// insert - Insert the specified key/value pair into the map. If the key
/// already exists in the map, return false and ignore the request, otherwise
/// insert it and return true.
diff --git a/llvm/unittests/ADT/StringMapTest.cpp b/llvm/unittests/ADT/StringMapTest.cpp
index 73c91f5fdd39..98fbd6e1df5a 100644
--- a/llvm/unittests/ADT/StringMapTest.cpp
+++ b/llvm/unittests/ADT/StringMapTest.cpp
@@ -387,6 +387,70 @@ TEST_F(StringMapTest, MoveAssignment) {
ASSERT_EQ(B.count("x"), 0u);
}
+TEST_F(StringMapTest, EqualEmpty) {
+ StringMap<int> A;
+ StringMap<int> B;
+ ASSERT_TRUE(A == B);
+ ASSERT_FALSE(A != B);
+ ASSERT_TRUE(A == A); // self check
+}
+
+TEST_F(StringMapTest, EqualWithValues) {
+ StringMap<int> A;
+ A["A"] = 1;
+ A["B"] = 2;
+ A["C"] = 3;
+ A["D"] = 3;
+
+ StringMap<int> B;
+ B["A"] = 1;
+ B["B"] = 2;
+ B["C"] = 3;
+ B["D"] = 3;
+
+ ASSERT_TRUE(A == B);
+ ASSERT_TRUE(B == A);
+ ASSERT_FALSE(A != B);
+ ASSERT_FALSE(B != A);
+ ASSERT_TRUE(A == A); // self check
+}
+
+TEST_F(StringMapTest, NotEqualMissingKeys) {
+ StringMap<int> A;
+ A["A"] = 1;
+ A["B"] = 2;
+
+ StringMap<int> B;
+ B["A"] = 1;
+ B["B"] = 2;
+ B["C"] = 3;
+ B["D"] = 3;
+
+ ASSERT_FALSE(A == B);
+ ASSERT_FALSE(B == A);
+ ASSERT_TRUE(A != B);
+ ASSERT_TRUE(B != A);
+}
+
+TEST_F(StringMapTest, NotEqualWithDifferentValues) {
+ StringMap<int> A;
+ A["A"] = 1;
+ A["B"] = 2;
+ A["C"] = 100;
+ A["D"] = 3;
+
+ StringMap<int> B;
+ B["A"] = 1;
+ B["B"] = 2;
+ B["C"] = 3;
+ B["D"] = 3;
+
+ ASSERT_FALSE(A == B);
+ ASSERT_FALSE(B == A);
+ ASSERT_TRUE(A != B);
+ ASSERT_TRUE(B != A);
+}
+
struct Countable {
int &InstanceCount;
int Number;
More information about the cfe-commits
mailing list