[PATCH] D126974: [scan-build-py] Fix exception on shutdown with sarif-html output format

[Daniel via Phabricator via cfe-commits]

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

lgtm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126974

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

[PATCH] D107887: scan-build-py: Force the opening in utf-8

[Daniel via Phabricator via cfe-commits]

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

Seems reasonable to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107887

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

[PATCH] D101139: Create install targets for scan-build-py.

[Daniel via Phabricator via cfe-commits]

isthismyaccount updated this revision to Diff 351308.
isthismyaccount added a comment.

Rebased to hopefully resolve merge issues.


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

https://reviews.llvm.org/D101139

Files:
  clang/cmake/caches/Fuchsia-stage2.cmake
  clang/tools/CMakeLists.txt
  clang/tools/scan-build-py/CMakeLists.txt
  clang/tools/scan-build-py/bin/analyze-build
  clang/tools/scan-build-py/bin/analyze-c++
  clang/tools/scan-build-py/bin/analyze-cc
  clang/tools/scan-build-py/bin/intercept-build
  clang/tools/scan-build-py/bin/intercept-c++
  clang/tools/scan-build-py/bin/intercept-cc
  clang/tools/scan-build-py/bin/scan-build
  clang/tools/scan-build-py/lib/libear/__init__.py
  clang/tools/scan-build-py/lib/libear/config.h.in
  clang/tools/scan-build-py/lib/libear/ear.c
  clang/tools/scan-build-py/lib/libscanbuild/__init__.py
  clang/tools/scan-build-py/lib/libscanbuild/analyze.py
  clang/tools/scan-build-py/lib/libscanbuild/arguments.py
  clang/tools/scan-build-py/lib/libscanbuild/clang.py
  clang/tools/scan-build-py/lib/libscanbuild/compilation.py
  clang/tools/scan-build-py/lib/libscanbuild/intercept.py
  clang/tools/scan-build-py/lib/libscanbuild/report.py
  clang/tools/scan-build-py/lib/libscanbuild/resources/scanview.css
  clang/tools/scan-build-py/lib/libscanbuild/resources/selectable.js
  clang/tools/scan-build-py/lib/libscanbuild/resources/sorttable.js
  clang/tools/scan-build-py/lib/libscanbuild/shell.py
  clang/tools/scan-build-py/libear/__init__.py
  clang/tools/scan-build-py/libear/config.h.in
  clang/tools/scan-build-py/libear/ear.c
  clang/tools/scan-build-py/libexec/analyze-c++
  clang/tools/scan-build-py/libexec/analyze-cc
  clang/tools/scan-build-py/libexec/intercept-c++
  clang/tools/scan-build-py/libexec/intercept-cc
  clang/tools/scan-build-py/libscanbuild/__init__.py
  clang/tools/scan-build-py/libscanbuild/analyze.py
  clang/tools/scan-build-py/libscanbuild/arguments.py
  clang/tools/scan-build-py/libscanbuild/clang.py
  clang/tools/scan-build-py/libscanbuild/compilation.py
  clang/tools/scan-build-py/libscanbuild/intercept.py
  clang/tools/scan-build-py/libscanbuild/report.py
  clang/tools/scan-build-py/libscanbuild/resources/scanview.css
  clang/tools/scan-build-py/libscanbuild/resources/selectable.js
  clang/tools/scan-build-py/libscanbuild/resources/sorttable.js
  clang/tools/scan-build-py/libscanbuild/shell.py
  clang/tools/scan-build-py/tests/__init__.py
  clang/tools/scan-build-py/tests/functional/cases/__init__.py
  clang/tools/scan-build-py/tests/functional/cases/test_exec_anatomy.py
  clang/tools/scan-build-py/tests/functional/cases/test_from_cdb.py
  clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py
  clang/tools/scan-build-py/tests/unit/test_analyze.py

Index: clang/tools/scan-build-py/tests/unit/test_analyze.py
===
--- clang/tools/scan-build-py/tests/unit/test_analyze.py
+++ clang/tools/scan-build-py/tests/unit/test_analyze.py
@@ -18,9 +18,9 @@
 # scan-build can be easily matched up to compare results.
 def test_directory_name_comparison(self):
 with libear.TemporaryDirectory() as tmpdir, \
- sut.report_directory(tmpdir, False) as report_dir1, \
- sut.report_directory(tmpdir, False) as report_dir2, \
- sut.report_directory(tmpdir, False) as report_dir3:
+ sut.report_directory(tmpdir, False, 'html') as report_dir1, \
+ sut.report_directory(tmpdir, False, 'html') as report_dir2, \
+ sut.report_directory(tmpdir, False, 'html') as report_dir3:
 self.assertLess(report_dir1, report_dir2)
 self.assertLess(report_dir2, report_dir3)
 
Index: clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py
===
--- clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py
+++ clang/tools/scan-build-py/tests/functional/cases/test_from_cmd.py
@@ -17,7 +17,7 @@
 @staticmethod
 def run_analyzer(outdir, args, cmd):
 return check_call_and_report(
-['scan-build', '--intercept-first', '-o', outdir] + args,
+['scan-build-py', '--intercept-first', '-o', outdir] + args,
 cmd)
 
 def test_regular_keeps_report_dir(self):
@@ -49,7 +49,7 @@
 with libear.TemporaryDirectory() as tmpdir:
 make = make_args(tmpdir) + ['build_regular']
 outdir = check_call_and_report(
-['scan-build', '--plist', '-o', tmpdir, '--override-compiler'],
+['scan-build-py', '--plist', '-o', tmpdir, '--override-compiler'],
 make)
 
 self.assertTrue(os.path.isdir(outdir))
@@ -59,7 +59,7 @@
 with libear.TemporaryDirectory() as tmpdir:
 make = make_args(tmpdir) + ['build_regular']
 outdir = check_call_and_report(
-['scan-build', '--plist', '-

[PATCH] D101139: Create install targets for scan-build-py.

[Daniel via Phabricator via cfe-commits]

isthismyaccount commandeered this revision.
isthismyaccount edited reviewers, added: aabbaabb; removed: isthismyaccount.
isthismyaccount added a comment.

Grabbing this revision.


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

https://reviews.llvm.org/D101139

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

[PATCH] D101139: Create install targets for scan-build-py.

[Daniel via Phabricator via cfe-commits]

isthismyaccount added a comment.

Thanks for the reviews. I'll take over this and try to get the merge conflicts 
resolved.


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

https://reviews.llvm.org/D101139

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

[PATCH] D96163: [analyzer] Add 12.0.0. release notes

[Daniel via Phabricator via cfe-commits]

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

LGTM on my part.


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

https://reviews.llvm.org/D96163

___
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 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 &InstanceCount;
   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 &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.
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) {
+  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-c

[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

[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

[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 &InstanceCount;
   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 &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.
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) {
+  v.push_back(Foo());
+  // CHECK-FIXES: v.emplace_back();
+  // CHECK-MESSAGES: warning: use emplace_back instead of push_back [hicpp-use-emplace,modernize-use-emp

[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

[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 &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;
+  }
+
   /// 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) {
+  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
===
--- clang-tools-extra/clang-tidy/Clang

[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 &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;
+  }
+
   /// 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) {
+  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 co

[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 &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;
+  }
+
   /// 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) {
+  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;
+  

[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

[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) {
+  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 &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::removeDuplicatedFixesOfAliasCheckers() {
+  using UniqueErrorSet =
+  std::set;
+  UniqueErrorSet UniqueErrors;
+  for (auto &Error : Errors) {
+std::pair Inserted =
+UniqueErrors.insert(&Error);
+
+// 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) {
+  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 &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::removeDuplicatedFixesOfAliasCheckers() {
+  using UniqueErrorSet =
+  std::set;
+  UniqueErrorSet UniqueErrors;
+  for (auto &Error : Errors) {
+std::pair Inserted =
+UniqueErrors.insert(&Error);
+
+// 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 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) {
+  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 &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::removeDuplicatedFixesOfAliasCheckers() {
+  std::set
+  UniqueErrors;
+  for (auto &Error : Errors) {
+auto Inserted = UniqueErrors.insert(&Error);
+
+// 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] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

Daniel599 added a comment.

In D68539#1727503 , @aaron.ballman 
wrote:

> Thank you for the patch! I've commit on your behalf in 
> e477988309dbde214a6d16ec690a416882714aac 
> 


Thank you for the commit,
just a little question. shouldn't the test file 
`clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp`
 be added?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539



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

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

Daniel599 added a comment.

In D68539#1723629 , @aaron.ballman 
wrote:

> Do you need someone to commit this on your behalf?


Yes I would like to.
Thank you :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539



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

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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

removed curly braces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
  clang/include/clang/Basic/IdentifierTable.h

Index: clang/include/clang/Basic/IdentifierTable.h
===
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -581,6 +581,8 @@
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
   /// Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ParameterCase, value: lower_case} \
+// RUN:   ]}'
+
+int func(int Break) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'; cannot be fixed because 'break' would conflict with a keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+// CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
+
+#define foo 3
+int func2(int Foo) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for parameter 'Foo'; cannot be fixed because 'foo' would conflict with a macro definition
+  // CHECK-FIXES: {{^}}int func2(int Foo) {{{$}}
+  if (Foo == 1) {
+// CHECK-FIXES: {{^}}  if (Foo == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -65,6 +65,24 @@
 std::string Suffix;
   };
 
+  /// This enum will be used in %select of the diagnostic message.
+  /// Each value below IgnoreFailureThreshold should have an error message.
+  enum class ShouldFixStatus {
+ShouldFix,
+ConflictsWithKeyword, /// The fixup will conflict with a language keyword,
+  /// so we can't fix it automatically.
+ConflictsWithMacroDefinition, /// The fixup will conflict with a macro
+  /// definition, so we can't fix it
+  /// automatically.
+
+/// Values pass this threshold will be ignored completely
+/// i.e no message, no fixup.
+IgnoreFailureThreshold,
+
+InsideMacro, /// If the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+  };
+
   /// Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
@@ -76,13 +94,19 @@
 ///
 /// ie: if the identifier was used or declared within a macro we won't offer
 /// a fixup for safety reasons.
-bool ShouldFix;
+bool ShouldFix() const { return FixStatus == ShouldFixStatus::ShouldFix; }
+
+bool ShouldNotify() const {
+  return FixStatus < ShouldFixStatus::IgnoreFailureThreshold;
+}
+
+ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
 
 /// A set of all the identifier usages starting SourceLocation, in
 /// their encoded form.
 llvm::DenseSet RawUsageLocs;
 
-NamingCheckFailure() : ShouldFix(true) {}
+NamingCheckFailure() = default;
   };
 
   typedef std::pair NamingCheckId;
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -691,10 +691,11 @@
   if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
 return;
 
-  if (!Failure.ShouldFix)
+  if (!Failure.ShouldFix())
 return;
 
-  Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr);
+  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+Failure.FixStatus = IdentifierNamingCheck::ShouldFixStatus::InsideMacro;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
@@ -873,6 +874

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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

Hi, any updates regarding my patch? issues to fix?
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539



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

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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

Added check for macro definition, please re-review the enum `ShouldFixStatus` 
as now I use it as a switch-case within the diagnostic message.
Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
  clang/include/clang/Basic/IdentifierTable.h

Index: clang/include/clang/Basic/IdentifierTable.h
===
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -581,6 +581,8 @@
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
   /// Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
@@ -0,0 +1,27 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ParameterCase, value: lower_case} \
+// RUN:   ]}'
+
+int func(int Break) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'; cannot be fixed because 'break' would conflict with a keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+// CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
+
+#define foo 3
+int func2(int Foo) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: invalid case style for parameter 'Foo'; cannot be fixed because 'foo' would conflict with a macro definition
+  // CHECK-FIXES: {{^}}int func2(int Foo) {{{$}}
+  if (Foo == 1) {
+// CHECK-FIXES: {{^}}  if (Foo == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -64,6 +64,24 @@
 std::string Suffix;
   };
 
+  /// This enum will be used in %select of the diagnostic message.
+  /// Each value below IgnoreFailureThreshold should have an error message.
+  enum class ShouldFixStatus {
+ShouldFix,
+ConflictsWithKeyword, /// The fixup will conflict with a language keyword,
+  /// so we can't fix it automatically.
+ConflictsWithMacroDefinition, /// The fixup will conflict with a macro
+  /// definition, so we can't fix it
+  /// automatically.
+
+/// Values pass this threshold will be ignored completely
+/// i.e no message, no fixup.
+IgnoreFailureThreshold,
+
+InsideMacro, /// If the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+  };
+
   /// Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
@@ -75,13 +93,19 @@
 ///
 /// ie: if the identifier was used or declared within a macro we won't offer
 /// a fixup for safety reasons.
-bool ShouldFix;
+bool ShouldFix() const { return FixStatus == ShouldFixStatus::ShouldFix; }
+
+bool ShouldNotify() const {
+  return FixStatus < ShouldFixStatus::IgnoreFailureThreshold;
+}
+
+ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
 
 /// A set of all the identifier usages starting SourceLocation, in
 /// their encoded form.
 llvm::DenseSet RawUsageLocs;
 
-NamingCheckFailure() : ShouldFix(true) {}
+NamingCheckFailure() = default;
   };
 
   typedef std::pair NamingCheckId;
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -679,10 +679,11 @@
   if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
 return;
 
-  if (!Failure.ShouldFix)
+  if (!Failure.ShouldFix())
 return;
 
-  Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr);
+  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+Failure.FixStatus = Identifi

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:868
+  if (CheckNewIdentifier != Idents.end() &&
+  CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;

Daniel599 wrote:
> aaron.ballman wrote:
> > What if changing it would switch to using a macro instead of a keyword? 
> > e.g.,
> > ```
> > #define foo 12
> > 
> > void func(int Foo); // Changing Foo to foo would be bad, no?
> > ```
> That's another type of bug, just like the one I found 
> https://bugs.llvm.org/show_bug.cgi?id=43306
> I don't aim on solving all of them in one patch, my patch just fixes keywords.
> Also, I don't think my patch makes the above situation worse.
Regarding your comment about macro name, I can fix it using 
`IdentifierInfo::hasMacroDefinition`.
Should I fix it in this patch? I`ll add another value to `ShouldFixStatus` and 
another error message


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539



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

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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

code-review fixes


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
  clang/include/clang/Basic/IdentifierTable.h

Index: clang/include/clang/Basic/IdentifierTable.h
===
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -581,6 +581,8 @@
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
   /// Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ParameterCase, value: lower_case} \
+// RUN:   ]}'
+
+int func(int Break) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'; cannot be fixed because 'break' would conflict with a keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+// CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -64,6 +64,14 @@
 std::string Suffix;
   };
 
+  enum class ShouldFixStatus {
+ShouldFix,
+InsideMacro, /// If the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+ConflictsWithKeyword /// The fixup will conflict with a language keyword, so
+ /// we can't fix it automatically.
+  };
+
   /// Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
@@ -75,13 +83,20 @@
 ///
 /// ie: if the identifier was used or declared within a macro we won't offer
 /// a fixup for safety reasons.
-bool ShouldFix;
+bool ShouldFix() const { return (FixStatus == ShouldFixStatus::ShouldFix); }
+
+bool ShouldNotify() const {
+  return FixStatus == ShouldFixStatus::ShouldFix ||
+ FixStatus == ShouldFixStatus::ConflictsWithKeyword;
+}
+
+ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
 
 /// A set of all the identifier usages starting SourceLocation, in
 /// their encoded form.
 llvm::DenseSet RawUsageLocs;
 
-NamingCheckFailure() : ShouldFix(true) {}
+NamingCheckFailure() = default;
   };
 
   typedef std::pair NamingCheckId;
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -679,10 +679,11 @@
   if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
 return;
 
-  if (!Failure.ShouldFix)
+  if (!Failure.ShouldFix())
 return;
 
-  Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr);
+  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+Failure.FixStatus = IdentifierNamingCheck::ShouldFixStatus::InsideMacro;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
@@ -861,6 +862,13 @@
   DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
   .getSourceRange();
 
+  const IdentifierTable &Idents = Decl->getASTContext().Idents;
+  auto CheckNewIdentifier = Idents.find(Fixup);
+  if (CheckNewIdentifier != Idents.end() &&
+  CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
+  }
+
   Failure.Fixup = std::move(Fixup);
   Failure.KindName = std::move(KindName);
   addUsage(NamingCheckFailures, Decl, Range);
@@ -923,24 +931,31 @@
 if (Failure.KindName.empty())
   continue;
 
-if (Failure.ShouldFix) {
-  auto Diag = diag(Decl.first, "invalid case style for %0 '%1'")
-  << Failu

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:868
+  if (CheckNewIdentifier != Idents.end() &&
+  CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;

aaron.ballman wrote:
> What if changing it would switch to using a macro instead of a keyword? e.g.,
> ```
> #define foo 12
> 
> void func(int Foo); // Changing Foo to foo would be bad, no?
> ```
That's another type of bug, just like the one I found 
https://bugs.llvm.org/show_bug.cgi?id=43306
I don't aim on solving all of them in one patch, my patch just fixes keywords.
Also, I don't think my patch makes the above situation worse.



Comment at: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h:88
+
+bool ShouldNotify() const {
+  return (FixStatus == ShouldFixStatus::ShouldFix ||

aaron.ballman wrote:
> This seems a bit confusing to me. It seems like the reason to not generate a 
> fixit is being used to determine whether to diagnose at all, which seems 
> incorrect despite sort of being what the old code did.
I`m sorry, I didn't understand you.
I tried to keep the old behavior of "cannot fix inside macro" and that's why I 
needed the method `ShouldNotify`.
Do you suggest another idea or other code flow?



Comment at: clang/include/clang/Basic/IdentifierTable.h:584
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+

aaron.ballman wrote:
> Is this actually needed? Pretty sure you can use `std::find(table.begin(), 
> table.end(), Name);` at the call site (or something similar).
Yes, because std::find would be slower here compare to `HashTable.find`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539



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

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

Daniel599 added a comment.

In D68539#1699288 , @MyDeveloperDay 
wrote:

> > I saw this issue when I submitted mine 
> > (https://bugs.llvm.org/show_bug.cgi?id=43306) which for now seems to be a 
> > harder fix, still looking for a solution there.
>
> oh boy!...are you gonna have to look at every variable/macro in scope and 
> compare?


Don't know yet, I thought about using  Sema::CheckShadow or Sema::LookupName, 
but I can't find a Sema instance, not sure how those methods effect performance


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539



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

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

Daniel599 added a comment.

In D68539#1696864 , @MyDeveloperDay 
wrote:

> I logged the original bug and I like it!
>
> I think the warning is better than mutating with a prefix, Thank you.
>
> I'll let the code owners approve it, but you have my vote!


glad to hear it :)
I saw this issue when I submitted mine 
(https://bugs.llvm.org/show_bug.cgi?id=43306) which for now seems to be a 
harder fix, still looking for solution there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539



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

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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

added -U99 to diff cmd


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
  clang/include/clang/Basic/IdentifierTable.h

Index: clang/include/clang/Basic/IdentifierTable.h
===
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -581,6 +581,8 @@
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
   /// Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ParameterCase, value: lower_case} \
+// RUN:   ]}'
+
+int func(int Break) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'. Cannot be fixed because 'break' would conflict with a language keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+// CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -64,6 +64,14 @@
 std::string Suffix;
   };
 
+  enum class ShouldFixStatus {
+ShouldFix,
+InsideMacro, /// if the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+ConflictsWithKeyword /// The fixup will conflict with a language keyword, so
+ /// we can't fix it automatically
+  };
+
   /// Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
@@ -75,13 +83,20 @@
 ///
 /// ie: if the identifier was used or declared within a macro we won't offer
 /// a fixup for safety reasons.
-bool ShouldFix;
+bool ShouldFix() const { return (FixStatus == ShouldFixStatus::ShouldFix); }
+
+bool ShouldNotify() const {
+  return (FixStatus == ShouldFixStatus::ShouldFix ||
+  FixStatus == ShouldFixStatus::ConflictsWithKeyword);
+}
+
+ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
 
 /// A set of all the identifier usages starting SourceLocation, in
 /// their encoded form.
 llvm::DenseSet RawUsageLocs;
 
-NamingCheckFailure() : ShouldFix(true) {}
+NamingCheckFailure() = default;
   };
 
   typedef std::pair NamingCheckId;
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -679,10 +679,11 @@
   if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
 return;
 
-  if (!Failure.ShouldFix)
+  if (!Failure.ShouldFix())
 return;
 
-  Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr);
+  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+Failure.FixStatus = IdentifierNamingCheck::ShouldFixStatus::InsideMacro;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
@@ -861,6 +862,13 @@
   DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
   .getSourceRange();
 
+  IdentifierTable &Idents = Decl->getASTContext().Idents;
+  auto CheckNewIdentifier = Idents.find(Fixup);
+  if (CheckNewIdentifier != Idents.end() &&
+  CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
+  }
+
   Failure.Fixup = std::move(Fixup);
   Failure.KindName = std::move(KindName);
   addUsage(NamingCheckFailures, Decl, Range);
@@ -923,24 +931,35 @@
 if (Failure.KindName.empty())
   continue;
 
-if (Failure.ShouldFix) {
-  auto Diag = diag(Decl.first, "invalid case style

[PATCH] D68539: [clang-tidy] fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

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

code fixes according to code-review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D68539

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
  clang/include/clang/Basic/IdentifierTable.h

Index: clang/include/clang/Basic/IdentifierTable.h
===
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -581,6 +581,8 @@
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
   /// Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ParameterCase, value: lower_case} \
+// RUN:   ]}'
+
+int func(int Break) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'. Cannot be fixed because 'break' would conflict with a language keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+// CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -64,6 +64,14 @@
 std::string Suffix;
   };
 
+  enum class ShouldFixStatus {
+ShouldFix,
+InsideMacro, /// if the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+ConflictsWithKeyword /// The fixup will conflict with a language keyword, so
+ /// we can't fix it automatically
+  };
+
   /// Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
@@ -75,13 +83,20 @@
 ///
 /// ie: if the identifier was used or declared within a macro we won't offer
 /// a fixup for safety reasons.
-bool ShouldFix;
+bool ShouldFix() const { return (FixStatus == ShouldFixStatus::ShouldFix); }
+
+bool ShouldNotify() const {
+  return (FixStatus == ShouldFixStatus::ShouldFix ||
+  FixStatus == ShouldFixStatus::ConflictsWithKeyword);
+}
+
+ShouldFixStatus FixStatus = ShouldFixStatus::ShouldFix;
 
 /// A set of all the identifier usages starting SourceLocation, in
 /// their encoded form.
 llvm::DenseSet RawUsageLocs;
 
-NamingCheckFailure() : ShouldFix(true) {}
+NamingCheckFailure() = default;
   };
 
   typedef std::pair NamingCheckId;
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -679,10 +679,11 @@
   if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
 return;
 
-  if (!Failure.ShouldFix)
+  if (!Failure.ShouldFix())
 return;
 
-  Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr);
+  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+Failure.FixStatus = IdentifierNamingCheck::ShouldFixStatus::InsideMacro;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
@@ -861,6 +862,13 @@
   DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
   .getSourceRange();
 
+  IdentifierTable &Idents = Decl->getASTContext().Idents;
+  auto CheckNewIdentifier = Idents.find(Fixup);
+  if (CheckNewIdentifier != Idents.end() &&
+  CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+Failure.FixStatus = ShouldFixStatus::ConflictsWithKeyword;
+  }
+
   Failure.Fixup = std::move(Fixup);
   Failure.KindName = std::move(KindName);
   addUsage(NamingCheckFailures, Decl, Range);
@@ -923,24 +931,35 @@
 if (Failure.KindName.empty())
   continue;
 
-if (Failure.ShouldFix) {
-  auto Diag = diag(Decl.first, "

[PATCH] D68539: fix for readability-identifier-naming incorrectly fixes variables which become keywords

[Daniel via Phabricator via cfe-commits]

Daniel599 created this revision.
Daniel599 added reviewers: llvm-commits, alexfh, alexfh_.
Daniel599 added projects: clang-tools-extra, LLVM.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
Daniel599 edited the summary of this revision.

This patch fixes 'Bug 41120'
https://bugs.llvm.org/show_bug.cgi?id=41120
When clang-tidy encounter a fixup name that would become a keyword, it just 
display a warning with reason and doesn't attempt to fix it.
Please let me know if this patch is OK, Thanks.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68539

Files:
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
  clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
  
clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
  clang/include/clang/Basic/IdentifierTable.h

Index: clang/include/clang/Basic/IdentifierTable.h
===
--- clang/include/clang/Basic/IdentifierTable.h
+++ clang/include/clang/Basic/IdentifierTable.h
@@ -581,6 +581,8 @@
   iterator end() const   { return HashTable.end(); }
   unsigned size() const  { return HashTable.size(); }
 
+  iterator find(StringRef Name) const { return HashTable.find(Name); }
+
   /// Print some statistics to stderr that indicate how well the
   /// hashing is doing.
   void PrintStats() const;
Index: clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
===
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/readability-identifier-naming-bugfix-name-conflicts.cpp
@@ -0,0 +1,15 @@
+// RUN: %check_clang_tidy %s readability-identifier-naming %t -- \
+// RUN:   -config='{CheckOptions: [ \
+// RUN: {key: readability-identifier-naming.ParameterCase, value: lower_case} \
+// RUN:   ]}'
+
+int func(int Break) {
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: invalid case style for parameter 'Break'. Cannot be fixed because 'break' would conflict with a language keyword
+  // CHECK-FIXES: {{^}}int func(int Break) {{{$}}
+  if (Break == 1) {
+// CHECK-FIXES: {{^}}  if (Break == 1) {{{$}}
+return 2;
+  }
+
+  return 0;
+}
\ No newline at end of file
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.h
@@ -64,6 +64,14 @@
 std::string Suffix;
   };
 
+  enum class ShouldFixStatus {
+ShouldFix = 0,
+InsideMacro, /// if the identifier was used or declared within a macro we
+ /// won't offer a fixup for safety reasons.
+ConflictsWithKeyword /// The fixup will conflict with a language keyword, so
+ /// we can't fix it automatically
+  };
+
   /// Holds an identifier name check failure, tracking the kind of the
   /// identifer, its possible fixup and the starting locations of all the
   /// identifier usages.
@@ -75,13 +83,20 @@
 ///
 /// ie: if the identifier was used or declared within a macro we won't offer
 /// a fixup for safety reasons.
-bool ShouldFix;
+bool ShouldFix() const { return (FixStatus == ShouldFixStatus::ShouldFix); }
+
+bool ShouldNotify() const {
+  return (FixStatus == ShouldFixStatus::ShouldFix ||
+  FixStatus == ShouldFixStatus::ConflictsWithKeyword);
+}
+
+ShouldFixStatus FixStatus;
 
 /// A set of all the identifier usages starting SourceLocation, in
 /// their encoded form.
 llvm::DenseSet RawUsageLocs;
 
-NamingCheckFailure() : ShouldFix(true) {}
+NamingCheckFailure() : FixStatus(ShouldFixStatus::ShouldFix) {}
   };
 
   typedef std::pair NamingCheckId;
Index: clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
===
--- clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
+++ clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp
@@ -679,10 +679,11 @@
   if (!Failure.RawUsageLocs.insert(FixLocation.getRawEncoding()).second)
 return;
 
-  if (!Failure.ShouldFix)
+  if (!Failure.ShouldFix())
 return;
 
-  Failure.ShouldFix = utils::rangeCanBeFixed(Range, SourceMgr);
+  if (!utils::rangeCanBeFixed(Range, SourceMgr))
+Failure.FixStatus = IdentifierNamingCheck::ShouldFixStatus::InsideMacro;
 }
 
 /// Convenience method when the usage to be added is a NamedDecl
@@ -861,6 +862,13 @@
   DeclarationNameInfo(Decl->getDeclName(), Decl->getLocation())
   .getSourceRange();
 
+  auto &Idents = Decl->getASTContext().Idents;
+  auto CheckNewIdentifier = Idents.find(Fixup);
+  if (CheckNewIdentifier != Idents.end() &&
+  CheckNewIdentifier->second->isKeyword(getLangOpts())) {
+Failure.FixStatus = Sho