[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Nathan James via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGaf4f2eb47636: [clang-tidy] remove duplicate fixes of alias 
checkers (authored by Daniel599, committed by njames93).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  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-conflicted-fixes-of-alias-checkers.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
  llvm/include/llvm/ADT/StringMap.h
  llvm/unittests/ADT/StringMapTest.cpp

Index: llvm/unittests/ADT/StringMapTest.cpp
===
--- llvm/unittests/ADT/StringMapTest.cpp
+++ llvm/unittests/ADT/StringMapTest.cpp
@@ -387,6 +387,70 @@
   ASSERT_EQ(B.count("x"), 0u);
 }
 
+TEST_F(StringMapTest, EqualEmpty) {
+  StringMap A;
+  StringMap B;
+  ASSERT_TRUE(A == B);
+  ASSERT_FALSE(A != B);
+  ASSERT_TRUE(A == A); // self check
+}
+
+TEST_F(StringMapTest, EqualWithValues) {
+  StringMap A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 3;
+  A["D"] = 3;
+
+  StringMap 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 A;
+  A["A"] = 1;
+  A["B"] = 2;
+
+  StringMap 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 A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 100;
+  A["D"] = 3;
+
+  StringMap 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 
   int Number;
Index: llvm/include/llvm/ADT/StringMap.h
===
--- llvm/include/llvm/ADT/StringMap.h
+++ llvm/include/llvm/ADT/StringMap.h
@@ -248,6 +248,26 @@
 return count(MapEntry.getKey());
   }
 
+  /// equal - check whether both of the containers are equal.
+  bool operator==(const StringMap ) const {
+if (size() != RHS.size())
+  return false;
+
+for (const auto  : *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 ) 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.
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
+++ 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;
 }
 
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ 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 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  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 ) {
+  v.push_back(Foo());
+  // CHECK-FIXES: v.emplace_back();
+  // 

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 updated this revision to Diff 272052.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  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-conflicted-fixes-of-alias-checkers.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
  llvm/include/llvm/ADT/StringMap.h
  llvm/unittests/ADT/StringMapTest.cpp

Index: llvm/unittests/ADT/StringMapTest.cpp
===
--- llvm/unittests/ADT/StringMapTest.cpp
+++ llvm/unittests/ADT/StringMapTest.cpp
@@ -387,6 +387,70 @@
   ASSERT_EQ(B.count("x"), 0u);
 }
 
+TEST_F(StringMapTest, EqualEmpty) {
+  StringMap A;
+  StringMap B;
+  ASSERT_TRUE(A == B);
+  ASSERT_FALSE(A != B);
+  ASSERT_TRUE(A == A); // self check
+}
+
+TEST_F(StringMapTest, EqualWithValues) {
+  StringMap A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 3;
+  A["D"] = 3;
+
+  StringMap 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 A;
+  A["A"] = 1;
+  A["B"] = 2;
+
+  StringMap 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 A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 100;
+  A["D"] = 3;
+
+  StringMap 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 
   int Number;
Index: llvm/include/llvm/ADT/StringMap.h
===
--- llvm/include/llvm/ADT/StringMap.h
+++ llvm/include/llvm/ADT/StringMap.h
@@ -248,6 +248,26 @@
 return count(MapEntry.getKey());
   }
 
+  /// equal - check whether both of the containers are equal.
+  bool operator==(const StringMap ) const {
+if (size() != RHS.size())
+  return false;
+
+for (const auto  : *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 ) 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.
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
+++ 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;
 }
 
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ 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 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  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 ) {
+  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]
+}
+
Index: 

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 marked 3 inline comments as done.
Daniel599 added a comment.

I fixed all as your comments suggested, thanks :)
If this patch is ready, can someone commit it on my behalf please? (I don't 
have write permissions)
Thank you.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Nathan James via Phabricator via cfe-commits]

njames93 accepted this revision.
njames93 added a comment.

LG, I have nothing else to add here.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

LGTM aside from a few small nits, but please wait a bit for @njames93 in case 
they have final thoughts.




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:758
+
+// Unique error, we keep it and move along
+if (Inserted.second) {

Add full stops to the end of your comments (here and elsewhere).



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:777
+  "or make sure they are both configured the same. "
+  "Aliased checkers: '{0}', '{1}'",
+  ExistingError.DiagnosticName, Error.DiagnosticName)

Daniel599 wrote:
> njames93 wrote:
> > You don't really need the Aliased checkers note as that is wrapped in the 
> > initial diagnostic message. 
> > Also if there was a check that could spew out 3 different fix-its for the 
> > same diagnostic, this would result in the duplication note being made 
> > twice, maybe the notes should be cleared too?
> regarding your comment about 3 fix-it, as we walked before, there isn't a 
> case like that (I didn't find any) as I wanted to make a test out of it.
> I added the last line as it would show who are the two that conflict 
> (supporting the future case of more than 2 aliased checkers),
> I can remove it if it doesn't help, let me know.
> 
> > maybe the notes should be cleared too?
> I am open for suggestions about how to re-write this message :)
> I also think it could be better
> 
I would reword it to be a run-on sentence: `cannot apply fix-it because an 
alias checker has suggested a different fix-it; please remove one of the 
checkers ('{0}', '{1}') or ensure they are both configured the same`

This also nicely removes the nit about terminating punctuation in a diagnostic.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:250
   void removeIncompatibleErrors();
+  void removeDuplicatedFixesOfAliasCheckers();
 

Daniel599 wrote:
> maybe I need to rename this method since now it's removing the errors also, 
> I`ll do it when I get back from work.
I think this should still be renamed. How about 
`removeDuplicatedDiagnosticsOfAliasCheckers()`?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 marked an inline comment as done.
Daniel599 added a comment.

ping?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 added a comment.

Any additional changes/fixes regarding this patch?
Thanks.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 marked an inline comment as done.
Daniel599 added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:777
+  "or make sure they are both configured the same. "
+  "Aliased checkers: '{0}', '{1}'",
+  ExistingError.DiagnosticName, Error.DiagnosticName)

njames93 wrote:
> You don't really need the Aliased checkers note as that is wrapped in the 
> initial diagnostic message. 
> Also if there was a check that could spew out 3 different fix-its for the 
> same diagnostic, this would result in the duplication note being made twice, 
> maybe the notes should be cleared too?
regarding your comment about 3 fix-it, as we walked before, there isn't a case 
like that (I didn't find any) as I wanted to make a test out of it.
I added the last line as it would show who are the two that conflict 
(supporting the future case of more than 2 aliased checkers),
I can remove it if it doesn't help, let me know.

> maybe the notes should be cleared too?
I am open for suggestions about how to re-write this message :)
I also think it could be better



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:777
+  "or make sure they are both configured the same. "
+  "Aliased checkers: '{0}', '{1}'",
+  ExistingError.DiagnosticName, Error.DiagnosticName)

You don't really need the Aliased checkers note as that is wrapped in the 
initial diagnostic message. 
Also if there was a check that could spew out 3 different fix-its for the same 
diagnostic, this would result in the duplication note being made twice, maybe 
the notes should be cleared too?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 updated this revision to Diff 268055.
Daniel599 added a comment.

Added `StringMap::operator!=` and also unit tests


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  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-conflicted-fixes-of-alias-checkers.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
  llvm/include/llvm/ADT/StringMap.h
  llvm/unittests/ADT/StringMapTest.cpp

Index: llvm/unittests/ADT/StringMapTest.cpp
===
--- llvm/unittests/ADT/StringMapTest.cpp
+++ llvm/unittests/ADT/StringMapTest.cpp
@@ -387,6 +387,70 @@
   ASSERT_EQ(B.count("x"), 0u);
 }
 
+TEST_F(StringMapTest, EqualEmpty) {
+  StringMap A;
+  StringMap B;
+  ASSERT_TRUE(A == B);
+  ASSERT_FALSE(A != B);
+  ASSERT_TRUE(A == A); // self check
+}
+
+TEST_F(StringMapTest, EqualWithValues) {
+  StringMap A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 3;
+  A["D"] = 3;
+
+  StringMap 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 A;
+  A["A"] = 1;
+  A["B"] = 2;
+
+  StringMap 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 A;
+  A["A"] = 1;
+  A["B"] = 2;
+  A["C"] = 100;
+  A["D"] = 3;
+
+  StringMap 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 
   int Number;
Index: llvm/include/llvm/ADT/StringMap.h
===
--- llvm/include/llvm/ADT/StringMap.h
+++ llvm/include/llvm/ADT/StringMap.h
@@ -248,6 +248,26 @@
 return count(MapEntry.getKey());
   }
 
+  /// equal - check whether both of the containers are equal
+  bool operator==(const StringMap ) const {
+if (size() != RHS.size())
+  return false;
+
+for (const auto  : *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 ) 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.
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
+++ 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;
 }
 
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ 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 
+class vector {
+public:
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  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 ) {
+  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]
+}
+
Index: 

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp:4-8
+template 
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};

This isn't needed for the test case and can safely be removed.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp:13-21
+  vector() = default;
+  vector(initializer_list) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 

The 2 constructors can be removed as well as the destructor.



Comment at: llvm/include/llvm/ADT/StringMap.h:251-268
+  /// equal - check whether both of the containers are equal
+  bool operator==(const StringMap ) const {
+if (size() != RHS.size())
+  return false;
+
+for (const auto  : *this) {
+  auto FindInRHS = RHS.find(KeyValue.getKey());

This needs unit tests `llvm/unittests/ADT/StringMapTest.cpp`, also a small nit 
but could you add the corresponding `operator!=`


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 updated this revision to Diff 267489.
Daniel599 marked an inline comment as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  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-conflicted-fixes-of-alias-checkers.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
  llvm/include/llvm/ADT/StringMap.h

Index: llvm/include/llvm/ADT/StringMap.h
===
--- llvm/include/llvm/ADT/StringMap.h
+++ llvm/include/llvm/ADT/StringMap.h
@@ -248,6 +248,24 @@
 return count(MapEntry.getKey());
   }
 
+  /// equal - check whether both of the containers are equal
+  bool operator==(const StringMap ) const {
+if (size() != RHS.size())
+  return false;
+
+for (const auto  : *this) {
+  auto FindInRHS = RHS.find(KeyValue.getKey());
+
+  if (FindInRHS == RHS.end())
+return false;
+
+  if (!(KeyValue.getValue() == FindInRHS->getValue()))
+return false;
+}
+
+return true;
+  }
+
   /// 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.
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
+++ 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;
 }
 
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
@@ -0,0 +1,48 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+template 
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};
+
+template 
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+  ~vector();
+};
+} // 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 ) {
+  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]
+}
+
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ 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 different fix-it, please remove one of the checkers or make sure they are both configured the same. Aliased checkers: '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;
+};
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- 

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:250
   void removeIncompatibleErrors();
+  void removeDuplicatedFixesOfAliasCheckers();
 

maybe I need to rename this method since now it's removing the errors also, 
I`ll do it when I get back from work.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 marked 2 inline comments as done.
Daniel599 added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:51
   bool IsWarningAsError;
+  std::vector EnabledDiagnosticAliases;
 };

njames93 wrote:
> Just ignore this, but do you ever get so bored and feel the need to save 24 
> bytes... https://gist.github.com/njames93/aac662ca52d345245e5e6f5dbc47d484 :)
That's a really cool trick, good to know :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D80753#2065067 , @Daniel599 wrote:

> Thank you @njames93, I already started and took a bit different approach.
>  In case of a conflict, leaving it to clang-tidy itself didn't help as it 
> added both of the fix-its together resulting in `= 0{};`, so I thought it 
> will be better to disable both of them.


Fair enough, your solution looks a lot nicer than what I suggested anyway.

> Sadly I didn't find 3 aliased checkers which can be configured to produce 
> different code so I can't check this edge case.
>  Please let me know if another changes are needed for this patch

You could create a unit test for 3 aliased checks with different options, 
however if there isn't a check that users can use to reproduce that edge case 
there is no point in worrying about it.




Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:765-778
+  (*Inserted.first)->Message.Fix;
+
+  if (!(CandidateFix == ExistingFix)) {
+std::string AliasedCheckerFixConflict =
+"cannot apply fix-it because an alias checker has "
+"suggested a different fix-it, please remove one of the checkers "
+"or make sure they are both configured the same. "

`s/(*Inserted.first)->/ExistingError.`



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:779
+(*Inserted.first)
+->Notes.emplace_back(std::move(AliasedCheckerFixConflict));
+  }

```
ExistingError.Notes.emplace_back(llvm::formatv(
"cannot apply fix-it because an alias checker has "
"suggested a different fix-it, please remove one of the checkers "
"or make sure they are both configured the same. "
"Aliased checkers: '{0}', '{1}'",
ExistingError.DiagnosticName, Error.DiagnosticName).str());```



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h:51
   bool IsWarningAsError;
+  std::vector EnabledDiagnosticAliases;
 };

Just ignore this, but do you ever get so bored and feel the need to save 24 
bytes... https://gist.github.com/njames93/aac662ca52d345245e5e6f5dbc47d484 :)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 updated this revision to Diff 267478.
Daniel599 added a comment.

Thank you @njames93, I already started and took a bit different approach.
In case of a conflict, leaving it to clang-tidy itself didn't help as it added 
both of the fix-its together resulting in `= 0{};`, so I thought it will be 
better to disable both of them.
Sadly I didn't find 3 aliased checkers which can be configured to produce 
different code so I can't check this edge case.
Please let me know if another changes are needed for this patch


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  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-conflicted-fixes-of-alias-checkers.cpp
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
  clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
  llvm/include/llvm/ADT/StringMap.h

Index: llvm/include/llvm/ADT/StringMap.h
===
--- llvm/include/llvm/ADT/StringMap.h
+++ llvm/include/llvm/ADT/StringMap.h
@@ -248,6 +248,24 @@
 return count(MapEntry.getKey());
   }
 
+  /// equal - check whether both of the containers are equal
+  bool operator==(const StringMap ) const {
+if (size() != RHS.size())
+  return false;
+
+for (const auto  : *this) {
+  auto FindInRHS = RHS.find(KeyValue.getKey());
+
+  if (FindInRHS == RHS.end())
+return false;
+
+  if (!(KeyValue.getValue() == FindInRHS->getValue()))
+return false;
+}
+
+return true;
+  }
+
   /// 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.
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
===
--- clang-tools-extra/test/clang-tidy/infrastructure/duplicate-reports.cpp
+++ 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;
 }
 
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
@@ -0,0 +1,48 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+template 
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};
+
+template 
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+  ~vector();
+};
+} // 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 ) {
+  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]
+}
+
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ 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 different fix-it, please remove one of the checkers or make sure they are both configured the 

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

In D80753#2064857 , @Daniel599 wrote:

> I have made the needed changes in order to detect a conflict in duplicated 
> fix-it of aliased checkers and also added a test for it.
>  Now I`ll try to work on combining aliased errors together,  any tips 
> regarding this feature?


Here is a quick draft I built up for this ClangTidyDiagnosticsConsumer.cpp 
 lmk what 
you think.
Some of the handling probably can be changed for conflicting fix its.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 updated this revision to Diff 267455.
Daniel599 added a comment.
Herald added subscribers: llvm-commits, dexonsmith.
Herald added a project: LLVM.

I have made the needed changes in order to detect a conflict in duplicated 
fix-it of aliased checkers and also added a test for it.
Now I`ll try to work on combining aliased errors together,  any tips regarding 
this feature?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  
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
  llvm/include/llvm/ADT/StringMap.h

Index: llvm/include/llvm/ADT/StringMap.h
===
--- llvm/include/llvm/ADT/StringMap.h
+++ llvm/include/llvm/ADT/StringMap.h
@@ -248,6 +248,24 @@
 return count(MapEntry.getKey());
   }
 
+  /// equal - check whether both of the containers are equal
+  bool operator==(const StringMap ) const {
+if (size() != RHS.size())
+  return false;
+
+for (const auto  : *this) {
+  auto FindInRHS = RHS.find(KeyValue.getKey());
+
+  if (FindInRHS == RHS.end())
+return false;
+
+  if (!(KeyValue.getValue() == FindInRHS->getValue()))
+return false;
+}
+
+return true;
+  }
+
   /// 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.
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
@@ -0,0 +1,52 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+template 
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};
+
+template 
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+  ~vector();
+};
+} // namespace std
+
+class Foo {
+public:
+  Foo() : _num1(0)
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init]
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [hicpp-member-init]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'cppcoreguidelines-pro-type-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 ) {
+  v.push_back(Foo());
+  // CHECK-FIXES: v.emplace_back();
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace]
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'hicpp-use-emplace'
+}
+
Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/duplicate-conflicted-fixes-of-alias-checkers.cpp
@@ -0,0 +1,25 @@
+// 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]
+  // CHECK-MESSAGES: note: cannot apply fix-it because an alias checker has suggested a different fix-it, please remove one of the checkers or make sure they are both configured the same. Aliased checkers: 'cppcoreguidelines-pro-type-member-init', 'hicpp-member-init'
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [hicpp-member-init]
+  // CHECK-MESSAGES: note: cannot apply fix-it because an alias checker has suggested a different fix-it, please remove one of the checkers or make sure they are both configured the same. Aliased checkers: 'cppcoreguidelines-pro-type-member-init', 'hicpp-member-init'
+  {
+_num1 = 10;
+  }
+
+  int use_the_members() const {
+return _num1 + _num2;
+  }
+
+private:

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 added a comment.

In D80753#2062586 , @aaron.ballman 
wrote:

> In D80753#2061565 , @njames93 wrote:
>
> > It may be worth verifying that the fix-its are identical too, multiple 
> > versions of a check could be running with differing options resulting in 
> > different fix-its generated, in that case we should let clang-tidy disable 
> > any conflicting fixes for us.
> >  Side note would it not be nicer to just group the diagnostics into one, 
> > thinking either of these ways
> >
> >   CHECK-MESSAGES: warning: use emplace_back instead of push_back 
> > [hicpp-use-emplace, modernize-use-emplace]
> >   CHECK-MESSAGES: warning: use emplace_back instead of push_back 
> > [hicpp-use-emplace] [modernize-use-emplace]
> >
> > This would result in cleaner diagnostics emitted and remove the need for 
> > that note.
>
>
> I think this is a nice approach. It also helps the case where a user sees a 
> false positive diagnostic and tries to disable it in their config file. Under 
> the removing duplicates behavior, the user would have a harder time 
> discovering what tests to disable, but under the above approach, they'd have 
> all the information they need up front.


**Regarding different fix-its generated: **
you are right, in order to fix it, I had to add `StringMap::operator ==` (soon 
I`ll update the diff), I hope it's OK.
I added the option cppcoreguidelines-pro-type-member-init.UseAssignment to 
check it, but the result I got was something like:

  int _num2 = 0{};

I think that if the fix-its aren't the same, both of them should be disabled 
with warning about it, what do you think?

**Regarding placing all errors of all aliases in one error:**
My original goal was to fix the bug that was introduce since clang-tidy-9, what 
you are suggesting sounds really cool, but I am still new to this code,
Any idea how can I implement such feature? also what should I do if the options 
of the alias checks aren't the same?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D80753#2061565 , @njames93 wrote:

> It may be worth verifying that the fix-its are identical too, multiple 
> versions of a check could be running with differing options resulting in 
> different fix-its generated, in that case we should let clang-tidy disable 
> any conflicting fixes for us.
>  Side note would it not be nicer to just group the diagnostics into one, 
> thinking either of these ways
>
>   CHECK-MESSAGES: warning: use emplace_back instead of push_back 
> [hicpp-use-emplace, modernize-use-emplace]
>   CHECK-MESSAGES: warning: use emplace_back instead of push_back 
> [hicpp-use-emplace] [modernize-use-emplace]
>
> This would result in cleaner diagnostics emitted and remove the need for that 
> note.


I think this is a nice approach. It also helps the case where a user sees a 
false positive diagnostic and tries to disable it in their config file. Under 
the removing duplicates behavior, the user would have a harder time discovering 
what tests to disable, but under the above approach, they'd have all the 
information they need up front.




Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp:3
+
+#include 
+namespace std {

Daniel599 wrote:
> Eugene.Zelenko wrote:
> > cstdio. Please also separate with newline.
> I wanted to use cstdio but the test fails with 'file not found', don't know 
> if it's specific on my host, anyway I`ll remove this include (and the printf)
 would have been wrong anyway -- clang-tidy (like clang) tests should 
not include system headers (unless they're faked system header so they're 
consistent across build bots).


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 updated this revision to Diff 267058.
Daniel599 marked an inline comment as not done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
@@ -0,0 +1,52 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+template 
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};
+
+template 
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+  ~vector();
+};
+} // namespace std
+
+class Foo {
+public:
+  Foo() : _num1(0)
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init]
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [hicpp-member-init]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'cppcoreguidelines-pro-type-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 ) {
+  v.push_back(Foo());
+  // CHECK-FIXES: v.emplace_back();
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace]
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'hicpp-use-emplace'
+}
+
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -246,6 +246,7 @@
 private:
   void finalizeLastError();
   void removeIncompatibleErrors();
+  void removeDuplicatedFixesOfAliasCheckers();
 
   /// Returns the \c HeaderFilter constructed for the options set in the
   /// context.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -634,6 +634,8 @@
 std::tuple Priority;
   };
 
+  removeDuplicatedFixesOfAliasCheckers();
+
   // Compute error sizes.
   std::vector Sizes;
   std::vector<
@@ -728,3 +730,36 @@
 removeIncompatibleErrors();
   return std::move(Errors);
 }
+
+namespace {
+struct LessClangTidyErrorWithoutDiagnosticName {
+  bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS) const {
+const tooling::DiagnosticMessage  = LHS->Message;
+const tooling::DiagnosticMessage  = RHS->Message;
+
+return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
+   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+  }
+};
+} // end anonymous namespace
+
+void ClangTidyDiagnosticConsumer::removeDuplicatedFixesOfAliasCheckers() {
+  using UniqueErrorSet =
+  std::set;
+  UniqueErrorSet UniqueErrors;
+  for (auto  : Errors) {
+std::pair Inserted =
+UniqueErrors.insert();
+
+// If we already have an error like this, just with the different
+// DiagnosticName, remove its Fix since we don't need same fix twice
+if (!Inserted.second && !Error.Message.Fix.empty()) {
+  Error.Message.Fix.clear();
+  std::string FixAlreadyExists =
+  "this fix will not be applied because an alias checker has already "
+  "provided it, see '" +
+  (*Inserted.first)->DiagnosticName + "'";
+  Error.Notes.emplace_back(std::move(FixAlreadyExists));
+}
+  }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 marked 4 inline comments as done.
Daniel599 added inline comments.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp:3
+
+#include 
+namespace std {

Eugene.Zelenko wrote:
> cstdio. Please also separate with newline.
I wanted to use cstdio but the test fails with 'file not found', don't know if 
it's specific on my host, anyway I`ll remove this include (and the printf)


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 updated this revision to Diff 267056.
Daniel599 marked an inline comment as done.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
@@ -0,0 +1,51 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+namespace std {
+template 
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};
+
+template 
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+  ~vector();
+};
+} // namespace std
+
+class Foo {
+public:
+  Foo() : _num1(0)
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init]
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [hicpp-member-init]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'cppcoreguidelines-pro-type-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 ) {
+  v.push_back(Foo());
+  // CHECK-FIXES: v.emplace_back();
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace]
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'hicpp-use-emplace'
+}
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -246,6 +246,7 @@
 private:
   void finalizeLastError();
   void removeIncompatibleErrors();
+  void removeDuplicatedFixesOfAliasCheckers();
 
   /// Returns the \c HeaderFilter constructed for the options set in the
   /// context.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -634,6 +634,8 @@
 std::tuple Priority;
   };
 
+  removeDuplicatedFixesOfAliasCheckers();
+
   // Compute error sizes.
   std::vector Sizes;
   std::vector<
@@ -728,3 +730,36 @@
 removeIncompatibleErrors();
   return std::move(Errors);
 }
+
+namespace {
+struct LessClangTidyErrorWithoutDiagnosticName {
+  bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS) const {
+const tooling::DiagnosticMessage  = LHS->Message;
+const tooling::DiagnosticMessage  = RHS->Message;
+
+return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
+   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+  }
+};
+} // end anonymous namespace
+
+void ClangTidyDiagnosticConsumer::removeDuplicatedFixesOfAliasCheckers() {
+  using UniqueErrorSet =
+  std::set;
+  UniqueErrorSet UniqueErrors;
+  for (auto  : Errors) {
+std::pair Inserted =
+UniqueErrors.insert();
+
+// If we already have an error like this, just with the different
+// DiagnosticName, remove its Fix since we don't need same fix twice
+if (!Inserted.second && !Error.Message.Fix.empty()) {
+  Error.Message.Fix.clear();
+  std::string FixAlreadyExists =
+  "this fix will not be applied because an alias checker has already "
+  "provided it, see '" +
+  (*Inserted.first)->DiagnosticName + "'";
+  Error.Notes.emplace_back(std::move(FixAlreadyExists));
+}
+  }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Nathan James via Phabricator via cfe-commits]

njames93 added a comment.

It may be worth verifying that the fix-its are identical too, multiple versions 
of a check could be running with differing options resulting in different 
fix-its generated, in that case we should let clang-tidy disable any 
conflicting fixes for us.
Side note would it not be nicer to just group the diagnostics into one, 
thinking either of these ways

  CHECK-MESSAGES: warning: use emplace_back instead of push_back 
[hicpp-use-emplace, modernize-use-emplace]
  CHECK-MESSAGES: warning: use emplace_back instead of push_back 
[hicpp-use-emplace] [modernize-use-emplace]

This would result in cleaner diagnostics emitted and remove the need for that 
note.




Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp:53
+}
\ No newline at end of file


Eugene.Zelenko wrote:
> Please add newline.
New line


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Eugene Zelenko via Phabricator via cfe-commits]

Eugene.Zelenko added inline comments.



Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:750
+  for (auto  : Errors) {
+auto Inserted = UniqueErrors.insert();
+

Please don't use auto when type is not spelled in same statement or not 
iterator.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp:3
+
+#include 
+namespace std {

cstdio. Please also separate with newline.



Comment at: 
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp:53
+}
\ No newline at end of file


Please add newline.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80753/new/

https://reviews.llvm.org/D80753



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D80753: [clang-tidy] remove duplicate fixes of alias checkers

[Daniel via Phabricator via cfe-commits]

Daniel599 created this revision.
Daniel599 added reviewers: alexfh, hokein.
Daniel599 added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, Charusso, xazax.hun.
Herald added a project: clang.

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


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D80753

Files:
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
  clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
  
clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp

Index: clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/infrastructure/duplicate-fixes-of-alias-checkers.cpp
@@ -0,0 +1,52 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-pro-type-member-init,hicpp-member-init,modernize-use-emplace,hicpp-use-emplace %t
+
+#include 
+namespace std {
+template 
+class initializer_list {
+public:
+  initializer_list() noexcept {}
+};
+
+template 
+class vector {
+public:
+  vector() = default;
+  vector(initializer_list) {}
+
+  void push_back(const T &) {}
+  void push_back(T &&) {}
+
+  template 
+  void emplace_back(Args &&... args){};
+  ~vector();
+};
+} // namespace std
+
+class Foo {
+public:
+  Foo() : _num1(0)
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [cppcoreguidelines-pro-type-member-init]
+  // CHECK-MESSAGES: warning: constructor does not initialize these fields: _num2 [hicpp-member-init]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'cppcoreguidelines-pro-type-member-init'
+  {
+printf("some code so it won't be a trival ctor\n");
+  }
+
+  int use_the_members() const {
+return _num1 + _num2;
+  }
+
+private:
+  int _num1;
+  int _num2;
+  // CHECK-FIXES: _num2{};
+};
+
+int should_use_emplace(std::vector ) {
+  v.push_back(Foo());
+  // CHECK-FIXES: v.emplace_back();
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace]
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [modernize-use-emplace]
+  // CHECK-MESSAGES: note: this fix will not be applied because an alias checker has already provided it, see 'hicpp-use-emplace'
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.h
@@ -246,6 +246,7 @@
 private:
   void finalizeLastError();
   void removeIncompatibleErrors();
+  void removeDuplicatedFixesOfAliasCheckers();
 
   /// Returns the \c HeaderFilter constructed for the options set in the
   /// context.
Index: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
===
--- clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
+++ clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp
@@ -634,6 +634,8 @@
 std::tuple Priority;
   };
 
+  removeDuplicatedFixesOfAliasCheckers();
+
   // Compute error sizes.
   std::vector Sizes;
   std::vector<
@@ -728,3 +730,34 @@
 removeIncompatibleErrors();
   return std::move(Errors);
 }
+
+namespace {
+struct LessClangTidyErrorWithoutDiagnosticName {
+  bool operator()(const ClangTidyError *LHS, const ClangTidyError *RHS) const {
+const tooling::DiagnosticMessage  = LHS->Message;
+const tooling::DiagnosticMessage  = RHS->Message;
+
+return std::tie(M1.FilePath, M1.FileOffset, M1.Message) <
+   std::tie(M2.FilePath, M2.FileOffset, M2.Message);
+  }
+};
+} // end anonymous namespace
+
+void ClangTidyDiagnosticConsumer::removeDuplicatedFixesOfAliasCheckers() {
+  std::set
+  UniqueErrors;
+  for (auto  : Errors) {
+auto Inserted = UniqueErrors.insert();
+
+// If we already have an error like this, just with the different
+// DiagnosticName, Remove its Fix since we don't need same fix twice
+if (!Inserted.second && !Error.Message.Fix.empty()) {
+  Error.Message.Fix.clear();
+  std::string FixAlreadyExists =
+  "this fix will not be applied because an alias checker has already "
+  "provided it, see '" +
+  (*Inserted.first)->DiagnosticName + "'";
+  Error.Notes.emplace_back(std::move(FixAlreadyExists));
+}
+  }
+}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits