[PATCH] D62404: [clang-tidy] Fix null pointer dereference in readability-identifier-naming

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: 
clang-tools-extra/clang-tidy/readability/IdentifierNamingCheck.cpp:804
+if (const auto *Typedef = TypePtr->getAs()) {
+  addUsage(NamingCheckFailures, Typedef->getDecl(),
+   Value->getSourceRange());

hokein wrote:
> We are risky of dereferencing a nullptr, as the `TypePtr` can be null now.
Are we not checking for null in the line where `TypePtr` gets declared? Or am I 
misunderstanding? 


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

https://reviews.llvm.org/D62404



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

[PATCH] D61861: DeleteNullPointerCheck now deletes until the end brace of the condition

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

This patch has now been merged. 
https://github.com/llvm/llvm-project/commit/bd324fa2273778430a4fdf8371fec5d64d2231bb


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61861



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

[PATCH] D61861: DeleteNullPointerCheck now deletes until the end brace of the condition

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

In D61861#1500900 , @J-Camilleri wrote:

> In D61861#1500868 , @madsravn wrote:
>
> > Looks good to me :)
>
>
> Thanks for the review and suggestion, who should I contact now in order to 
> push the patch?


I asked around and I was told that we should just leave a comment and then 
somebody will come around. I was also told that after a few patches you can 
obtain commit access if you want to push stuff yourself.

someone please commit this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61861



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

[PATCH] D61861: DeleteNullPointerCheck now deletes until the end brace of the condition

[Mads Ravn via Phabricator via cfe-commits]

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

Looks good to me :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61861



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

[PATCH] D61861: DeleteNullPointerCheck now deletes until the end brace of the condition

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

Besides my one comment this looks good to me.




Comment at: 
clang-tools-extra/test/clang-tidy/readability-delete-null-pointer.cpp:9
   // #1
-  if (p) { // #2
+  if (p /**/) { // #2
 delete p;

Would you mind creating a new test instead of changing the existing one? 
Removing tests could create another whole for a bug to fall through.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61861



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL301167: [clang-tidy] New check: 
modernize-replace-random-shuffle. (authored by madsravn).

Changed prior to commit:
  https://reviews.llvm.org/D30158?vs=96303=96362#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D30158

Files:
  clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  clang-tools-extra/trunk/test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/ModernizeTidyModule.cpp
@@ -19,6 +19,7 @@
 #include "RawStringLiteralCheck.h"
 #include "RedundantVoidArgCheck.h"
 #include "ReplaceAutoPtrCheck.h"
+#include "ReplaceRandomShuffleCheck.h"
 #include "ReturnBracedInitListCheck.h"
 #include "ShrinkToFitCheck.h"
 #include "UseAutoCheck.h"
@@ -54,6 +55,8 @@
 "modernize-redundant-void-arg");
 CheckFactories.registerCheck(
 "modernize-replace-auto-ptr");
+CheckFactories.registerCheck(
+"modernize-replace-random-shuffle");
 CheckFactories.registerCheck(
 "modernize-return-braced-init-list");
 CheckFactories.registerCheck("modernize-shrink-to-fit");
Index: clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/modernize/CMakeLists.txt
@@ -13,6 +13,7 @@
   RawStringLiteralCheck.cpp
   RedundantVoidArgCheck.cpp
   ReplaceAutoPtrCheck.cpp
+  ReplaceRandomShuffleCheck.cpp
   ReturnBracedInitListCheck.cpp
   ShrinkToFitCheck.cpp
   UseAutoCheck.cpp
Index: clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.h
+++ clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.h
@@ -0,0 +1,42 @@
+//===--- ReplaceRandomShuffleCheck.h - clang-tidy*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_RANDOM_SHUFFLE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_RANDOM_SHUFFLE_H
+
+#include "../ClangTidy.h"
+#include "../utils/IncludeInserter.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// std::random_shuffle will be removed as of C++17. This check will find and
+/// replace all occurrences of std::random_shuffle with std::shuffle.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-replace-random-shuffle.html
+class ReplaceRandomShuffleCheck : public ClangTidyCheck {
+public:
+  ReplaceRandomShuffleCheck(StringRef Name, ClangTidyContext *Context);
+  void registerPPCallbacks(CompilerInstance ) override;
+  void storeOptions(ClangTidyOptions::OptionMap ) override;
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult ) override;
+
+private:
+  std::unique_ptr IncludeInserter;
+  const utils::IncludeSorter::IncludeStyle IncludeStyle;
+};
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_REPLACE_RANDOM_SHUFFLE_H
Index: clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
@@ -0,0 +1,109 @@
+//===--- ReplaceRandomShuffleCheck.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "ReplaceRandomShuffleCheck.h"
+#include "../utils/FixItHintUtils.h"
+#include "clang/AST/ASTContext.h"
+#include 

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

In https://reviews.llvm.org/D30158#734810, @aaron.ballman wrote:

> This continues to LGTM; do you need someone to land the patch for you?


I will remove the extra newline and land the patch tomorrow. Thanks! :)


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 96303.
madsravn added a comment.

Small changes to code due to comments.


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+//CHECK-FIXES: #include 
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+using namespace std;
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+
+  std::shuffle(vec.begin(), vec.end());
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+  
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurrences of ``std::random_shuffle`` and replace it with ``std::shuffle``. In C++17 ``std::random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below are two examples of what kind of occurrences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both of these examples will be replaced with:
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
+
+One thing to be aware of here is that ``std::random_device`` is quite expensive to initialize. So if you are using the code in a performance critical place, you probably want to initialize it elsewhere. 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -124,6 +124,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -81,6 +81,11 @@
   Adds checks that implement the `High Integrity C++ Coding Standard `_ and other safety
   standards. Many checks are aliased to other modules.
 
+- New `modernize-replace-random-shuffle
+  

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

In https://reviews.llvm.org/D30158#702778, @jroelofs wrote:

> In https://reviews.llvm.org/D30158#702760, @madsravn wrote:
>
> > In https://reviews.llvm.org/D30158#699342, @jroelofs wrote:
> >
> > > In https://reviews.llvm.org/D30158#699132, @madsravn wrote:
> > >
> > > > In https://reviews.llvm.org/D30158#698871, @aaron.ballman wrote:
> > > >
> > > > > In https://reviews.llvm.org/D30158#696534, @madsravn wrote:
> > > > >
> > > > > > Any updates on this?
> > > > >
> > > > >
> > > > > Have you run it over the test suite on the revision before 
> > > > > random_shuffle was removed from libc++?
> > > >
> > > >
> > > > I can't find any more tests for random_shuffle. I have looked in the 
> > > > SVN log for from december 2014 until now. It works on the three tests 
> > > > currently in trunk.
> > >
> > >
> > > Try just before r294328.
> >
> >
> > I can't see any random_shuffle tests in the libc++ repo before r294328 
> > either. I don't know if they aren't there or if I just can't find them. 
> > Would you be able to point them out for me with revision and path?
>
>
> Did you look at the diff from the commit I mentioned? That's the one that 
> removed them.


I found them and I have tested them all. This check works fine on them.


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

In https://reviews.llvm.org/D30158#699342, @jroelofs wrote:

> In https://reviews.llvm.org/D30158#699132, @madsravn wrote:
>
> > In https://reviews.llvm.org/D30158#698871, @aaron.ballman wrote:
> >
> > > In https://reviews.llvm.org/D30158#696534, @madsravn wrote:
> > >
> > > > Any updates on this?
> > >
> > >
> > > Have you run it over the test suite on the revision before random_shuffle 
> > > was removed from libc++?
> >
> >
> > I can't find any more tests for random_shuffle. I have looked in the SVN 
> > log for from december 2014 until now. It works on the three tests currently 
> > in trunk.
>
>
> Try just before r294328.


I can't see any random_shuffle tests in the libc++ repo before r294328 either. 
I don't know if they aren't there or if I just can't find them. Would you be 
able to point them out for me with revision and path?


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

In https://reviews.llvm.org/D30158#698871, @aaron.ballman wrote:

> In https://reviews.llvm.org/D30158#696534, @madsravn wrote:
>
> > Any updates on this?
>
>
> Have you run it over the test suite on the revision before random_shuffle was 
> removed from libc++?


I can't find any more tests for random_shuffle. I have looked in the SVN log 
for from december 2014 until now. It works on the three tests currently in 
trunk.


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

Any updates on this?


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

Looks good for the two tests the are for `random_shuffle` in llvm libc++.


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

In https://reviews.llvm.org/D30158#689904, @aaron.ballman wrote:

> Out of curiosity, have you run this over libc++ or libstdc++ test suites 
> involving `std::random_shuffle`? If so, were the results acceptable?


I haven't. Good idea. I will get onto that. I don't have either, so I will just 
fetch them and set them up.


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 90223.
madsravn added a comment.

Last small changes based on comments.


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+//CHECK-FIXES: #include 
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+using namespace std;
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+
+  std::shuffle(vec.begin(), vec.end());
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+  
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurrences of ``std::random_shuffle`` and replace it with ``std::shuffle``. In C++17 ``std::random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below are two examples of what kind of occurrences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both of these examples will be replaced with:
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
+
+One thing to be aware of here is that ``std::random_device`` is quite expensive to initialize. So if you are using the code in a performance critical place, you probably want to initialize it elsewhere. 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -110,6 +110,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -72,6 +72,11 @@
 
   Finds uses of inline assembler.
 
+- New `modernize-replace-random-shuffle
+  `_ check
+
+  Finds and fixes usage of ``std::random_shuffle`` as the 

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+  auto Diag = [=]() {
+std::string Message = ReplaceMessage;

aaron.ballman wrote:
> madsravn wrote:
> > aaron.ballman wrote:
> > > madsravn wrote:
> > > > aaron.ballman wrote:
> > > > > madsravn wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > Is there a reason this needs to capture everything by copy? Also, 
> > > > > > > no need for the empty parens. Actually, is the lambda even 
> > > > > > > necessary at all?
> > > > > > Is it OK to capture by reference then? Or how do we want it in 
> > > > > > llvm? 
> > > > > > 
> > > > > > We need the lambda, because first I need to create the diag with a 
> > > > > > message based on the count of arguments and then I need to find 
> > > > > > fixits based on the same count. Example: 
> > > > > > 
> > > > > > 
> > > > > > ```
> > > > > > string message = "Message for 2 arguments";
> > > > > > if(argumentCount == 3) {
> > > > > >   message = "Message for 3 arguments";
> > > > > > }
> > > > > > auto Diag = diag(startLoc(), message);
> > > > > > if(argumentCount == 3) {
> > > > > >   Diag << FixitHint::FixForThreeArguments();
> > > > > > } else {
> > > > > >   Diag << FixitHint::FixForTwoArguments();
> > > > > > }
> > > > > > ```
> > > > > > 
> > > > > > So the idea with the lambda is to avoid doing the same if-statement 
> > > > > > twice. 
> > > > > But you call the lambda immediately rather than store it and reuse 
> > > > > it? It seems like you should be able to hoist a `DiagnosticBuilder` 
> > > > > variable outside of the if statement and skip the lambda entirely.
> > > > I am not sure what you mean by this. Can you elaborate? Can you give a 
> > > > short example how I would hoist a `DiagnosticBuilder` out?
> > > > 
> > > > I think I tried something like that, but it was not an option. 
> > > It's entirely possible I'm missing something (I'm distracted with 
> > > meetings this week), but I was envisioning:
> > > ```
> > > DiagnosticBuilder Diag;
> > > if (MatchedCallExpr->getNumArgs() == 3) {
> > >   Diag =
> > >   diag(MatchedCallExpr->getLocStart(),
> > >"'std::random_shuffle' has been removed in C++17; use "
> > >"'std::shuffle' and an alternative random mechanism instead");
> > >   Diag << FixItHint::CreateReplacement(
> > >   MatchedArgumentThree->getSourceRange(),
> > >   "std::mt19937(std::random_device()())");
> > > } else {
> > >   Diag = diag(MatchedCallExpr->getLocStart(),
> > > "'std::random_shuffle' has been removed in C++17; use 
> > > "
> > > "'std::shuffle' instead");
> > >   Diag << FixItHint::CreateInsertion(
> > >   MatchedCallExpr->getRParenLoc(),
> > >   ", std::mt19937(std::random_device()())");
> > > }
> > > ```
> > The constructor for `DiagnosticBuilder` is private. So I cannot do that. 
> > The idea had crossed my mind, but I think the lambda expression is nicer to 
> > look at. 
> > 
> > Should I investigate if there is another way to hoist the 
> > `DiagnosticBuilder` out, like using `diag()` to make a dummy 
> > `DiagnosticBuilder` outside and then use the copy constructor to assign 
> > inside the if-statement? Or can we live with the lambda expression? 
> Ah, okay, that was the bit I was missing. Thank you for being patient. I 
> think the lambda (with the reference capture) is fine as-is.
> Thank you for being patient.

Right back at you. We are working towards the same goal after all :) 

For future reference: Should I try to avoid lambda expressions like this? 




https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+  auto Diag = [=]() {
+std::string Message = ReplaceMessage;

aaron.ballman wrote:
> madsravn wrote:
> > aaron.ballman wrote:
> > > madsravn wrote:
> > > > aaron.ballman wrote:
> > > > > Is there a reason this needs to capture everything by copy? Also, no 
> > > > > need for the empty parens. Actually, is the lambda even necessary at 
> > > > > all?
> > > > Is it OK to capture by reference then? Or how do we want it in llvm? 
> > > > 
> > > > We need the lambda, because first I need to create the diag with a 
> > > > message based on the count of arguments and then I need to find fixits 
> > > > based on the same count. Example: 
> > > > 
> > > > 
> > > > ```
> > > > string message = "Message for 2 arguments";
> > > > if(argumentCount == 3) {
> > > >   message = "Message for 3 arguments";
> > > > }
> > > > auto Diag = diag(startLoc(), message);
> > > > if(argumentCount == 3) {
> > > >   Diag << FixitHint::FixForThreeArguments();
> > > > } else {
> > > >   Diag << FixitHint::FixForTwoArguments();
> > > > }
> > > > ```
> > > > 
> > > > So the idea with the lambda is to avoid doing the same if-statement 
> > > > twice. 
> > > But you call the lambda immediately rather than store it and reuse it? It 
> > > seems like you should be able to hoist a `DiagnosticBuilder` variable 
> > > outside of the if statement and skip the lambda entirely.
> > I am not sure what you mean by this. Can you elaborate? Can you give a 
> > short example how I would hoist a `DiagnosticBuilder` out?
> > 
> > I think I tried something like that, but it was not an option. 
> It's entirely possible I'm missing something (I'm distracted with meetings 
> this week), but I was envisioning:
> ```
> DiagnosticBuilder Diag;
> if (MatchedCallExpr->getNumArgs() == 3) {
>   Diag =
>   diag(MatchedCallExpr->getLocStart(),
>"'std::random_shuffle' has been removed in C++17; use "
>"'std::shuffle' and an alternative random mechanism instead");
>   Diag << FixItHint::CreateReplacement(
>   MatchedArgumentThree->getSourceRange(),
>   "std::mt19937(std::random_device()())");
> } else {
>   Diag = diag(MatchedCallExpr->getLocStart(),
> "'std::random_shuffle' has been removed in C++17; use "
> "'std::shuffle' instead");
>   Diag << FixItHint::CreateInsertion(
>   MatchedCallExpr->getRParenLoc(),
>   ", std::mt19937(std::random_device()())");
> }
> ```
The constructor for `DiagnosticBuilder` is private. So I cannot do that. The 
idea had crossed my mind, but I think the lambda expression is nicer to look 
at. 

Should I investigate if there is another way to hoist the `DiagnosticBuilder` 
out, like using `diag()` to make a dummy `DiagnosticBuilder` outside and then 
use the copy constructor to assign inside the if-statement? Or can we live with 
the lambda expression? 


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

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



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+  auto Diag = [=]() {
+std::string Message = ReplaceMessage;

aaron.ballman wrote:
> madsravn wrote:
> > aaron.ballman wrote:
> > > Is there a reason this needs to capture everything by copy? Also, no need 
> > > for the empty parens. Actually, is the lambda even necessary at all?
> > Is it OK to capture by reference then? Or how do we want it in llvm? 
> > 
> > We need the lambda, because first I need to create the diag with a message 
> > based on the count of arguments and then I need to find fixits based on the 
> > same count. Example: 
> > 
> > 
> > ```
> > string message = "Message for 2 arguments";
> > if(argumentCount == 3) {
> >   message = "Message for 3 arguments";
> > }
> > auto Diag = diag(startLoc(), message);
> > if(argumentCount == 3) {
> >   Diag << FixitHint::FixForThreeArguments();
> > } else {
> >   Diag << FixitHint::FixForTwoArguments();
> > }
> > ```
> > 
> > So the idea with the lambda is to avoid doing the same if-statement twice. 
> But you call the lambda immediately rather than store it and reuse it? It 
> seems like you should be able to hoist a `DiagnosticBuilder` variable outside 
> of the if statement and skip the lambda entirely.
I am not sure what you mean by this. Can you elaborate? Can you give a short 
example how I would hoist a `DiagnosticBuilder` out?

I think I tried something like that, but it was not an option. 


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn marked 9 inline comments as done.
madsravn added inline comments.



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+  auto Diag = [=]() {
+std::string Message = ReplaceMessage;

aaron.ballman wrote:
> Is there a reason this needs to capture everything by copy? Also, no need for 
> the empty parens. Actually, is the lambda even necessary at all?
Is it OK to capture by reference then? Or how do we want it in llvm? 

We need the lambda, because first I need to create the diag with a message 
based on the count of arguments and then I need to find fixits based on the 
same count. Example: 


```
string message = "Message for 2 arguments";
if(argumentCount == 3) {
  message = "Message for 3 arguments";
}
auto Diag = diag(startLoc(), message);
if(argumentCount == 3) {
  Diag << FixitHint::FixForThreeArguments();
} else {
  Diag << FixitHint::FixForTwoArguments();
}
```

So the idea with the lambda is to avoid doing the same if-statement twice. 


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 90148.
madsravn added a comment.

Updated patch according to comments by Ballman.


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+//CHECK-FIXES: #include 
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+using namespace std;
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+
+  std::shuffle(vec.begin(), vec.end());
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+  
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: 'std::random_shuffle' has been removed in C++17; use 'std::shuffle' and an alternative random mechanism instead
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurrences of ``std::random_shuffle`` and replace it with ``std::shuffle``. In C++17 ``std::random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below are two examples of what kind of occurrences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both of these examples will be replaced with:
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
+
+One thing to be aware of here is that ``std::random_device`` is quite expensive to initialize. So if you are using the code in a performance critical place, you probably want to initialize it elsewhere. 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -110,6 +110,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -72,6 +72,11 @@
 
   Finds uses of inline assembler.
 
+- New `modernize-replace-random-shuffle
+  `_ check
+
+  Finds and fixes usage of ``std::random_shuffle`` 

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 89919.
madsravn added a comment.

Minor update to fix spelling mistake.


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+//CHECK-FIXES: #include 
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+using namespace std;
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+
+  std::shuffle(vec.begin(), vec.end());
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+  
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurrences of ``std::random_shuffle`` and replace it with ``std::shuffle``. In C++17 ``std::random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below is two examples of what kind of occurrences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both these examples will be replaced with
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
+
+One thing to be aware of here is that ``std::random_device`` is quite expensive to initialize. So if you are using the code in a performance critical place, you probably want to initialize it elsewhere. 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -110,6 +110,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -72,6 +72,11 @@
 
   Finds uses of inline assembler.
 
+- New 

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 89817.
madsravn marked an inline comment as done.
madsravn added a comment.

Made small changes based on comments.


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+//CHECK-FIXES: #include 
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+using namespace std;
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+
+  std::shuffle(vec.begin(), vec.end());
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+  
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurrences of ``std::random_shuffle`` and replace it with ``std::shuffle``. In C++17 ``std::random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below is two examples of what kind of occurrences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both these examples will be replaced with
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
+
+One thing to be aware of here is that ``std::random_device`` is quite expensive to initialize. So if you are using the code in a performance critical place, you probably want to initialize it elsewhere. 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -110,6 +110,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -72,6 +72,11 @@
 
   

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn marked 18 inline comments as done.
madsravn added inline comments.



Comment at: test/clang-tidy/modernize-replace-random-shuffle.cpp:50
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is 
deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is 
not usable for shuffle. You need to make additional changes if you want a 
specific random function
+  // CHECK-FIXES: shuffle(vec.begin(), vec.begin(), 
std::mt19937(std::random_device()()));
+

xazax.hun wrote:
> This check-fix might match the previous fix instead of this one. You might 
> want to make the fixes unique, e.g.: with a comment after a line. Note that 
> it is also worth to test that line ending comments are preserved.
> 
> Also, are you sure you want to do the fixit when a custom random function is 
> passed to random_shuffle?
I re-arranged them. This way the check-fixes does not say the same twice in a 
row. I am not sure what you mean by 'line ending comments are preserved'. Why 
shouldn't they be? 

The fixit should also be done when a custom random function is passed. 
random_shuffle will be removed and the signature of the custom random function 
will be changed. It's hard to do much differently than just issuing a warning.


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 89543.
madsravn added a comment.

Updated the code based on comments received.


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,58 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+//CHECK-FIXES: #include 
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+using namespace std;
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+
+  std::shuffle(vec.begin(), vec.end());
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+  
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,28 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurences of ``std::random_shuffle`` and replace it with ``std::shuffle``. In C++17 ``std::random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below is two examples of what kind of occurences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both these examples will be replaced with
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
+
+One thing to be aware of here is that ``std::random_device`` is quite expensive to initialize. So if you are using the code in a performance critical place, you probably want to initialize it elsewhere. 
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -110,6 +110,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -72,6 +72,11 @@
 
   Finds uses of inline assembler.
 
+- New 

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:81
+  Stream << "shuffle(";
+  FirstArgument->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+  Stream << ", ";

xazax.hun wrote:
> madsravn wrote:
> > xazax.hun wrote:
> > > madsravn wrote:
> > > > xazax.hun wrote:
> > > > > Wouldn't just using the original source range of the argument work?
> > > > > What about preserving the comments next to the arguments? 
> > > > I am not sure what you mean about the original source range. Can I just 
> > > > put that onto the Stream? 
> > > > 
> > > > Do you have an idea for the comments? Do you mean something like
> > > > 
> > > > 
> > > > ```
> > > > std::random_shuffle(
> > > >vec.begin(), // Comment
> > > >vec.end() // Comment
> > > > );
> > > > ```
> > > Or even:
> > > 
> > > 
> > > ```
> > > std::random_shuffle(vec.begin(), /*inlinecomment*/ vec.end());
> > > ```
> > Thanks for you other comments. Can you elaborate on how I might fix this?
> You might do a different strategy, like not touching the arguments at all, 
> but only inserting a new argument before the closing paren of the function 
> call. This way all the comments should be preserved. 
I could try that. So just change the name of the function, random_shuffle to 
shuffle, and then remove the third argument if present and insert third 
argument. I guess it will work, but it will make the code less elegant.


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:81
+  Stream << "shuffle(";
+  FirstArgument->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+  Stream << ", ";

xazax.hun wrote:
> madsravn wrote:
> > xazax.hun wrote:
> > > Wouldn't just using the original source range of the argument work?
> > > What about preserving the comments next to the arguments? 
> > I am not sure what you mean about the original source range. Can I just put 
> > that onto the Stream? 
> > 
> > Do you have an idea for the comments? Do you mean something like
> > 
> > 
> > ```
> > std::random_shuffle(
> >vec.begin(), // Comment
> >vec.end() // Comment
> > );
> > ```
> Or even:
> 
> 
> ```
> std::random_shuffle(vec.begin(), /*inlinecomment*/ vec.end());
> ```
Thanks for you other comments. Can you elaborate on how I might fix this?


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:30
+" The old user defined 'RandomFunction' is not usable for 'shuffle'. You "
+"need to "
+"make additional changes if you want a specific random function.";

xazax.hun wrote:
> Maybe it would be worth to reflow this literal.
It seems a little weird, but this is the result of clang-format. 



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:70
+  llvm::raw_string_ostream Stream(Buffer);
+  DRef->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+  StringRef StrRef(Stream.str());

xazax.hun wrote:
> What about accessing the buffer of the source file instead of pretty printing?
How would you do this? printPretty was the best that I could find fitting my 
needs. If you have something better fitting, please share :)



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:81
+  Stream << "shuffle(";
+  FirstArgument->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+  Stream << ", ";

xazax.hun wrote:
> Wouldn't just using the original source range of the argument work?
> What about preserving the comments next to the arguments? 
I am not sure what you mean about the original source range. Can I just put 
that onto the Stream? 

Do you have an idea for the comments? Do you mean something like


```
std::random_shuffle(
   vec.begin(), // Comment
   vec.end() // Comment
);
```



Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:101
+
+  auto Diag = diag(MatchedCallExpr->getLocStart(), Message);
+  Diag << FixItHint::CreateRemoval(MatchedCallExpr->getSourceRange());

xazax.hun wrote:
> You do not want to do fixits for code that is the result of macro expansions. 
Why not? And how would I fix that? 


https://reviews.llvm.org/D30158



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

[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.

[Mads Ravn via Phabricator via cfe-commits]

madsravn created this revision.
Herald added subscribers: JDevlieghere, mgorny.

random_shuffle was deprecated by C++14 and will be removed by C++17. This check 
will find and replace usage of random_shuffle with its modern counterpart 
(shuffle).


https://reviews.llvm.org/D30158

Files:
  clang-tidy/modernize/CMakeLists.txt
  clang-tidy/modernize/ModernizeTidyModule.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp
  clang-tidy/modernize/ReplaceRandomShuffleCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
  test/clang-tidy/modernize-replace-random-shuffle.cpp

Index: test/clang-tidy/modernize-replace-random-shuffle.cpp
===
--- test/clang-tidy/modernize-replace-random-shuffle.cpp
+++ test/clang-tidy/modernize-replace-random-shuffle.cpp
@@ -0,0 +1,55 @@
+// RUN: %check_clang_tidy %s modernize-replace-random-shuffle %t -- -- -std=c++11
+
+namespace std {
+template  struct vec_iterator {
+  T *ptr;
+  vec_iterator operator++(int);
+};
+
+template  struct vector {
+  typedef vec_iterator iterator;
+
+  iterator begin();
+  iterator end();
+};
+
+template 
+void random_shuffle(FwIt begin, FwIt end);
+
+template 
+void random_shuffle(FwIt begin, FwIt end, randomFunc& randomfunc);
+
+template 
+void shuffle(FwIt begin, FwIt end);
+} // namespace std
+
+// Random Func
+int myrandom (int i) { return i;}
+
+int main() {
+  std::vector vec;
+
+  std::random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.begin(), std::mt19937(std::random_device()()));
+
+  std::random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: std::shuffle(vec.begin(), vec.begin(), std::mt19937(std::random_device()()));
+
+  std::shuffle(vec.begin(), vec.end());
+
+  using namespace std;
+
+  random_shuffle(vec.begin(), vec.end());
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'
+  // CHECK-FIXES: shuffle(vec.begin(), vec.begin(), std::mt19937(std::random_device()()));
+
+  random_shuffle(vec.begin(), vec.end(), myrandom);
+  // CHECK-MESSAGE: [[@LINE-1]]:3: warning: do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'. The old user defined 'RandomFunction' is not usable for shuffle. You need to make additional changes if you want a specific random function
+  // CHECK-FIXES: shuffle(vec.begin(), vec.begin(), std::mt19937(std::random_device()()));
+
+  shuffle(vec.begin(), vec.end());
+
+  return 0;
+}
Index: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
===
--- docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
+++ docs/clang-tidy/checks/modernize-replace-random-shuffle.rst
@@ -0,0 +1,26 @@
+.. title:: clang-tidy - modernize-replace-random-shuffle
+
+modernize-replace-random-shuffle
+
+
+This check will find occurences of ``random_shuffle`` and replace it with ``shuffle``. In C++17 ``random_shuffle`` will no longer be available and thus we need to replace it.
+
+Below is two examples of what kind of occurences will be found and two examples of what it will be replaced with.
+
+.. code-block:: c++
+
+  std::vector v;
+
+  // First example
+  std::random_shuffle(vec.begin(), vec.end());
+
+  // Second example
+  std::random_shuffle(vec.begin(), vec.end(), randomFun);
+
+Both these examples will be replaced with
+
+.. code-block:: c++
+
+  std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
+The second example will also receive a warning that ``randomFunc`` is no longer supported in the same way as before so if the user wants the same functionality, the user will need to change the implementation of the ``randomFunc``.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -110,6 +110,7 @@
modernize-raw-string-literal
modernize-redundant-void-arg
modernize-replace-auto-ptr
+   modernize-replace-random-shuffle
modernize-return-braced-init-list
modernize-shrink-to-fit
modernize-use-auto
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -72,6 +72,11 @@
 
   Finds uses of inline assembler.
 
+- New `modernize-replace-random-shuffle
+  

[PATCH] D29726: [Clang-tidy] Fix for bug 31838: readability-delete-null-pointer does not work for class members

[Mads Ravn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL294912: [clang-tidy] Fix for bug 31838: 
readability-delete-null-pointer does not work… (authored by madsravn).

Changed prior to commit:
  https://reviews.llvm.org/D29726?vs=88123=88143#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29726

Files:
  clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp
  clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h
  clang-tools-extra/trunk/test/clang-tidy/readability-delete-null-pointer.cpp


Index: clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 to(decl(equalsBoundNode("deletedPointer"
   .bind("deleteExpr");
 
-  const auto PointerExpr =
-  ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer";
+  const auto DeleteMemberExpr =
+  cxxDeleteExpr(has(castExpr(has(memberExpr(hasDeclaration(
+
fieldDecl(equalsBoundNode("deletedMemberPointer"
+  .bind("deleteMemberExpr");
+
+  const auto PointerExpr = ignoringImpCasts(anyOf(
+  declRefExpr(to(decl().bind("deletedPointer"))),
+  memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer");
+
   const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean),
  hasSourceExpression(PointerExpr));
   const auto BinaryPointerCheckCondition =
   binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
  hasEitherOperand(PointerExpr));
 
   Finder->addMatcher(
   ifStmt(hasCondition(anyOf(PointerCondition, 
BinaryPointerCheckCondition)),
- hasThen(anyOf(DeleteExpr,
-   compoundStmt(has(DeleteExpr), statementCountIs(1))
-   .bind("compound"
+ hasThen(anyOf(
+ DeleteExpr, DeleteMemberExpr,
+ compoundStmt(has(anyOf(DeleteExpr, DeleteMemberExpr)),
+  statementCountIs(1))
+ .bind("compound"
   .bind("ifWithDelete"),
   this);
 }
Index: 
clang-tools-extra/trunk/test/clang-tidy/readability-delete-null-pointer.cpp
===
--- clang-tools-extra/trunk/test/clang-tidy/readability-delete-null-pointer.cpp
+++ clang-tools-extra/trunk/test/clang-tidy/readability-delete-null-pointer.cpp
@@ -59,6 +59,16 @@
   } else {
 c2 = c;
   }
+  struct A {
+void foo() {
+  if (mp) // #6
+delete mp;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: 'if' statement is 
unnecessary; deleting null pointer has no effect 
[readability-delete-null-pointer]
+  // CHECK-FIXES: {{^  }}// #6
+  // CHECK-FIXES-NEXT: delete mp;
+}
+int *mp;
+  };
 }
 
 void g() {


Index: clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 to(decl(equalsBoundNode("deletedPointer"
   .bind("deleteExpr");
 
-  const auto PointerExpr =
-  

[PATCH] D29726: [Clang-tidy] Fix for bug 31838: readability-delete-null-pointer does not work for class members

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 88123.
madsravn marked an inline comment as done.
madsravn added a comment.

Added comment in CHECK-FIX to ensure the line we are referring to is uniquely 
identified.


https://reviews.llvm.org/D29726

Files:
  clang-tidy/readability/DeleteNullPointerCheck.cpp
  clang-tidy/readability/DeleteNullPointerCheck.h
  test/clang-tidy/readability-delete-null-pointer.cpp


Index: test/clang-tidy/readability-delete-null-pointer.cpp
===
--- test/clang-tidy/readability-delete-null-pointer.cpp
+++ test/clang-tidy/readability-delete-null-pointer.cpp
@@ -59,6 +59,16 @@
   } else {
 c2 = c;
   }
+  struct A {
+void foo() {
+  if (mp) // #6
+delete mp;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: 'if' statement is 
unnecessary; deleting null pointer has no effect 
[readability-delete-null-pointer]
+  // CHECK-FIXES: {{^  }}// #6
+  // CHECK-FIXES-NEXT: delete mp;
+}
+int *mp;
+  };
 }
 
 void g() {
Index: clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 to(decl(equalsBoundNode("deletedPointer"
   .bind("deleteExpr");
 
-  const auto PointerExpr =
-  ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer";
+  const auto DeleteMemberExpr =
+  cxxDeleteExpr(has(castExpr(has(memberExpr(hasDeclaration(
+
fieldDecl(equalsBoundNode("deletedMemberPointer"
+  .bind("deleteMemberExpr");
+
+  const auto PointerExpr = ignoringImpCasts(anyOf(
+  declRefExpr(to(decl().bind("deletedPointer"))),
+  memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer");
+
   const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean),
  hasSourceExpression(PointerExpr));
   const auto BinaryPointerCheckCondition =
   binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
  hasEitherOperand(PointerExpr));
 
   Finder->addMatcher(
   ifStmt(hasCondition(anyOf(PointerCondition, 
BinaryPointerCheckCondition)),
- hasThen(anyOf(DeleteExpr,
-   compoundStmt(has(DeleteExpr), statementCountIs(1))
-   .bind("compound"
+ hasThen(anyOf(
+ DeleteExpr, DeleteMemberExpr,
+ compoundStmt(has(anyOf(DeleteExpr, DeleteMemberExpr)),
+  statementCountIs(1))
+ .bind("compound"
   .bind("ifWithDelete"),
   this);
 }


Index: test/clang-tidy/readability-delete-null-pointer.cpp
===
--- test/clang-tidy/readability-delete-null-pointer.cpp
+++ test/clang-tidy/readability-delete-null-pointer.cpp
@@ -59,6 +59,16 @@
   } else {
 c2 = c;
   }
+  struct A {
+void foo() {
+  if (mp) // #6
+delete mp;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: 'if' statement is unnecessary; deleting null pointer has no effect [readability-delete-null-pointer]
+  // CHECK-FIXES: {{^  }}// #6
+  // CHECK-FIXES-NEXT: delete mp;
+}
+int *mp;
+  };
 }
 
 void g() {
Index: clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 

[PATCH] D29726: [Clang-tidy] Fix for bug 31838: readability-delete-null-pointer does not work for class members

[Mads Ravn via Phabricator via cfe-commits]

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



Comment at: clang-tidy/readability/DeleteNullPointerCheck.cpp:46
+ DeleteExpr, DeleteMemberExpr,
+ compoundStmt(anyOf(has(DeleteExpr), has(DeleteMemberExpr)),
+  statementCountIs(1))

alexfh wrote:
> nit: Will `has(anyOf(DeleteExpr, DeleteMemberExpr))` work?
I could swear that I had tried that. It seems like the obvious way to do it. It 
worked, though.


https://reviews.llvm.org/D29726



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

[PATCH] D29726: [Clang-tidy] Fix for bug 31838: readability-delete-null-pointer does not work for class members

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 87829.
madsravn added a comment.

Small change to check.
Minor changes to the lit test.


https://reviews.llvm.org/D29726

Files:
  clang-tidy/readability/DeleteNullPointerCheck.cpp
  clang-tidy/readability/DeleteNullPointerCheck.h
  test/clang-tidy/readability-delete-null-pointer.cpp


Index: test/clang-tidy/readability-delete-null-pointer.cpp
===
--- test/clang-tidy/readability-delete-null-pointer.cpp
+++ test/clang-tidy/readability-delete-null-pointer.cpp
@@ -59,6 +59,16 @@
   } else {
 c2 = c;
   }
+  struct A {
+void foo() {
+  if (mp) // #6
+delete mp;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: 'if' statement is 
unnecessary; deleting null pointer has no effect 
[readability-delete-null-pointer]
+  // CHECK-FIXES: {{^  }}
+  // CHECK-FIXES-NEXT: delete mp;
+}
+int *mp;
+  };
 }
 
 void g() {
Index: clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 to(decl(equalsBoundNode("deletedPointer"
   .bind("deleteExpr");
 
-  const auto PointerExpr =
-  ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer";
+  const auto DeleteMemberExpr =
+  cxxDeleteExpr(has(castExpr(has(memberExpr(hasDeclaration(
+
fieldDecl(equalsBoundNode("deletedMemberPointer"
+  .bind("deleteMemberExpr");
+
+  const auto PointerExpr = ignoringImpCasts(anyOf(
+  declRefExpr(to(decl().bind("deletedPointer"))),
+  memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer");
+
   const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean),
  hasSourceExpression(PointerExpr));
   const auto BinaryPointerCheckCondition =
   binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
  hasEitherOperand(PointerExpr));
 
   Finder->addMatcher(
   ifStmt(hasCondition(anyOf(PointerCondition, 
BinaryPointerCheckCondition)),
- hasThen(anyOf(DeleteExpr,
-   compoundStmt(has(DeleteExpr), statementCountIs(1))
-   .bind("compound"
+ hasThen(anyOf(
+ DeleteExpr, DeleteMemberExpr,
+ compoundStmt(anyOf(has(DeleteExpr), has(DeleteMemberExpr)),
+  statementCountIs(1))
+ .bind("compound"
   .bind("ifWithDelete"),
   this);
 }


Index: test/clang-tidy/readability-delete-null-pointer.cpp
===
--- test/clang-tidy/readability-delete-null-pointer.cpp
+++ test/clang-tidy/readability-delete-null-pointer.cpp
@@ -59,6 +59,16 @@
   } else {
 c2 = c;
   }
+  struct A {
+void foo() {
+  if (mp) // #6
+delete mp;
+  // CHECK-MESSAGES: :[[@LINE-2]]:7: warning: 'if' statement is unnecessary; deleting null pointer has no effect [readability-delete-null-pointer]
+  // CHECK-FIXES: {{^  }}
+  // CHECK-FIXES-NEXT: delete mp;
+}
+int *mp;
+  };
 }
 
 void g() {
Index: clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 to(decl(equalsBoundNode("deletedPointer"
   .bind("deleteExpr");
 
-  const auto PointerExpr =
- 

[PATCH] D29726: [Clang-tidy] Fix for bug 31838: readability-delete-null-pointer does not work for class members

[Mads Ravn via Phabricator via cfe-commits]

madsravn created this revision.

Made a small fix to readability-delete-null-pointer check such that it includes 
class members.


https://reviews.llvm.org/D29726

Files:
  clang-tidy/readability/DeleteNullPointerCheck.cpp
  clang-tidy/readability/DeleteNullPointerCheck.h
  test/clang-tidy/readability-delete-null-pointer.cpp


Index: test/clang-tidy/readability-delete-null-pointer.cpp
===
--- test/clang-tidy/readability-delete-null-pointer.cpp
+++ test/clang-tidy/readability-delete-null-pointer.cpp
@@ -59,6 +59,16 @@
   } else {
 c2 = c;
   }
+  struct A {
+void foo() {
+  if (mp)
+delete mp;
+}
+int *mp;
+  };
+  // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: 'if' statement is unnecessary; 
deleting null pointer has no effect [readability-delete-null-pointer]
+
+  // CHECK-FIXES: delete mp;
 }
 
 void g() {
Index: clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on 
a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// 
http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 to(decl(equalsBoundNode("deletedPointer"
   .bind("deleteExpr");
 
-  const auto PointerExpr =
-  ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer";
+  const auto DeleteMemberExpr =
+  cxxDeleteExpr(has(castExpr(has(memberExpr(hasDeclaration(
+
fieldDecl(equalsBoundNode("deletedMemberPointer"
+  .bind("deleteMemberExpr");
+
+  const auto PointerExpr = ignoringImpCasts(anyOf(
+  declRefExpr(to(decl().bind("deletedPointer"))),
+  memberExpr(hasDeclaration(fieldDecl().bind("deletedMemberPointer");
+
   const auto PointerCondition = castExpr(hasCastKind(CK_PointerToBoolean),
  hasSourceExpression(PointerExpr));
   const auto BinaryPointerCheckCondition =
   binaryOperator(hasEitherOperand(castExpr(hasCastKind(CK_NullToPointer))),
  hasEitherOperand(PointerExpr));
 
   Finder->addMatcher(
   ifStmt(hasCondition(anyOf(PointerCondition, 
BinaryPointerCheckCondition)),
- hasThen(anyOf(DeleteExpr,
-   compoundStmt(has(DeleteExpr), statementCountIs(1))
-   .bind("compound"
+ hasThen(anyOf(
+ DeleteExpr, DeleteMemberExpr,
+ compoundStmt(anyOf(has(DeleteExpr), has(DeleteMemberExpr)),
+  statementCountIs(1))
+ .bind("compound"
   .bind("ifWithDelete"),
   this);
 }


Index: test/clang-tidy/readability-delete-null-pointer.cpp
===
--- test/clang-tidy/readability-delete-null-pointer.cpp
+++ test/clang-tidy/readability-delete-null-pointer.cpp
@@ -59,6 +59,16 @@
   } else {
 c2 = c;
   }
+  struct A {
+void foo() {
+  if (mp)
+delete mp;
+}
+int *mp;
+  };
+  // CHECK-MESSAGES: :[[@LINE-5]]:7: warning: 'if' statement is unnecessary; deleting null pointer has no effect [readability-delete-null-pointer]
+
+  // CHECK-FIXES: delete mp;
 }
 
 void g() {
Index: clang-tidy/readability/DeleteNullPointerCheck.h
===
--- clang-tidy/readability/DeleteNullPointerCheck.h
+++ clang-tidy/readability/DeleteNullPointerCheck.h
@@ -16,7 +16,8 @@
 namespace tidy {
 namespace readability {
 
-/// Check whether the 'if' statement is unnecessary before calling 'delete' on a pointer.
+/// Check whether the 'if' statement is unnecessary before calling 'delete' on a
+/// pointer.
 ///
 /// For the user-facing documentation see:
 /// http://clang.llvm.org/extra/clang-tidy/checks/readability-delete-null-pointer.html
Index: clang-tidy/readability/DeleteNullPointerCheck.cpp
===
--- clang-tidy/readability/DeleteNullPointerCheck.cpp
+++ clang-tidy/readability/DeleteNullPointerCheck.cpp
@@ -24,19 +24,28 @@
 to(decl(equalsBoundNode("deletedPointer"
   .bind("deleteExpr");
 
-  const auto PointerExpr =
-  ignoringImpCasts(declRefExpr(to(decl().bind("deletedPointer";
+  const auto DeleteMemberExpr 

[PATCH] D29018: [clang-tidy] Ignore implicit functions in performance-unnecessary-value-param

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

Looks good to me. Nice touch by solving with implicit.


https://reviews.llvm.org/D29018



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

[PATCH] D28614: [clang-tidy] Fix check for trivially copyable types in modernize-pass-by-value

[Mads Ravn via Phabricator via cfe-commits]

madsravn added a comment.

Looks good to me.


https://reviews.llvm.org/D28614



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

[PATCH] D28180: Fixing small errors in libAST Matcher Tutorial.

[Mads Ravn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL290766: [clang] Minor fix to libASTMatcherTutorial (authored 
by madsravn).

Changed prior to commit:
  https://reviews.llvm.org/D28180?vs=82735=82742#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28180

Files:
  cfe/trunk/docs/LibASTMatchersTutorial.rst


Index: cfe/trunk/docs/LibASTMatchersTutorial.rst
===
--- cfe/trunk/docs/LibASTMatchersTutorial.rst
+++ cfe/trunk/docs/LibASTMatchersTutorial.rst
@@ -496,9 +496,9 @@
 
   void LoopPrinter::run(const MatchFinder::MatchResult ) {
 ASTContext *Context = Result.Context;
-const ForStmt *FS = Result.Nodes.getStmtAs("forLoop");
+const ForStmt *FS = Result.Nodes.getNodeAs("forLoop");
 // We do not want to convert header files!
-if (!FS || 
!Context->getSourceManager().isFromMainFile(FS->getForLoc()))
+if (!FS || 
!Context->getSourceManager().isWrittenInMainFile(FS->getForLoc()))
   return;
 const VarDecl *IncVar = Result.Nodes.getNodeAs("incVarName");
 const VarDecl *CondVar = 
Result.Nodes.getNodeAs("condVarName");


Index: cfe/trunk/docs/LibASTMatchersTutorial.rst
===
--- cfe/trunk/docs/LibASTMatchersTutorial.rst
+++ cfe/trunk/docs/LibASTMatchersTutorial.rst
@@ -496,9 +496,9 @@
 
   void LoopPrinter::run(const MatchFinder::MatchResult ) {
 ASTContext *Context = Result.Context;
-const ForStmt *FS = Result.Nodes.getStmtAs("forLoop");
+const ForStmt *FS = Result.Nodes.getNodeAs("forLoop");
 // We do not want to convert header files!
-if (!FS || !Context->getSourceManager().isFromMainFile(FS->getForLoc()))
+if (!FS || !Context->getSourceManager().isWrittenInMainFile(FS->getForLoc()))
   return;
 const VarDecl *IncVar = Result.Nodes.getNodeAs("incVarName");
 const VarDecl *CondVar = Result.Nodes.getNodeAs("condVarName");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28180: Fixing small errors in libAST Matcher Tutorial.

[Mads Ravn via Phabricator via cfe-commits]

madsravn created this revision.
madsravn added reviewers: aaron.ballman, malcolm.parsons.
madsravn added a subscriber: cfe-commits.

The tutorial contains a few errors which results in code not being able to 
compile.

One error was described here: https://llvm.org/bugs/show_bug.cgi?id=25583 .

I found and fixed the error and one additional error.


https://reviews.llvm.org/D28180

Files:
  docs/LibASTMatchersTutorial.rst


Index: docs/LibASTMatchersTutorial.rst
===
--- docs/LibASTMatchersTutorial.rst
+++ docs/LibASTMatchersTutorial.rst
@@ -496,9 +496,9 @@
 
   void LoopPrinter::run(const MatchFinder::MatchResult ) {
 ASTContext *Context = Result.Context;
-const ForStmt *FS = Result.Nodes.getStmtAs("forLoop");
+const ForStmt *FS = Result.Nodes.getNodeAs("forLoop");
 // We do not want to convert header files!
-if (!FS || 
!Context->getSourceManager().isFromMainFile(FS->getForLoc()))
+if (!FS || 
!Context->getSourceManager().isWrittenInMainFile(FS->getForLoc()))
   return;
 const VarDecl *IncVar = Result.Nodes.getNodeAs("incVarName");
 const VarDecl *CondVar = 
Result.Nodes.getNodeAs("condVarName");


Index: docs/LibASTMatchersTutorial.rst
===
--- docs/LibASTMatchersTutorial.rst
+++ docs/LibASTMatchersTutorial.rst
@@ -496,9 +496,9 @@
 
   void LoopPrinter::run(const MatchFinder::MatchResult ) {
 ASTContext *Context = Result.Context;
-const ForStmt *FS = Result.Nodes.getStmtAs("forLoop");
+const ForStmt *FS = Result.Nodes.getNodeAs("forLoop");
 // We do not want to convert header files!
-if (!FS || !Context->getSourceManager().isFromMainFile(FS->getForLoc()))
+if (!FS || !Context->getSourceManager().isWrittenInMainFile(FS->getForLoc()))
   return;
 const VarDecl *IncVar = Result.Nodes.getNodeAs("incVarName");
 const VarDecl *CondVar = Result.Nodes.getNodeAs("condVarName");
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL290747: [clang-tidy] Add check 'misc-string-compare'. 
(authored by madsravn).

Changed prior to commit:
  https://reviews.llvm.org/D27210?vs=82521=82721#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27210

Files:
  clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
  clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/misc-string-compare.rst
  clang-tools-extra/trunk/test/clang-tidy/misc-string-compare.cpp

Index: clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.cpp
@@ -0,0 +1,82 @@
+//===--- MiscStringCompare.cpp - clang-tidy===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#include "StringCompareCheck.h"
+#include "../utils/FixItHintUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+static const StringRef CompareMessage = "do not use 'compare' to test equality "
+"of strings; use the string equality "
+"operator instead";
+
+void StringCompareCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus)
+return;
+
+  const auto StrCompare = cxxMemberCallExpr(
+  callee(cxxMethodDecl(hasName("compare"),
+   ofClass(classTemplateSpecializationDecl(
+   hasName("::std::basic_string"),
+  hasArgument(0, expr().bind("str2")), argumentCountIs(1),
+  callee(memberExpr().bind("str1")));
+
+  // First and second case: cast str.compare(str) to boolean.
+  Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(booleanType()),
+  has(StrCompare))
+ .bind("match1"),
+ this);
+
+  // Third and fourth case: str.compare(str) == 0 and str.compare(str) != 0.
+  Finder->addMatcher(
+  binaryOperator(anyOf(hasOperatorName("=="), hasOperatorName("!=")),
+ hasEitherOperand(StrCompare.bind("compare")),
+ hasEitherOperand(integerLiteral(equals(0)).bind("zero")))
+  .bind("match2"),
+  this);
+}
+
+void StringCompareCheck::check(const MatchFinder::MatchResult ) {
+  if (const auto *Matched = Result.Nodes.getNodeAs("match1")) {
+diag(Matched->getLocStart(), CompareMessage);
+return;
+  }
+
+  if (const auto *Matched = Result.Nodes.getNodeAs("match2")) {
+const ASTContext  = *Result.Context;
+
+if (const auto *Zero = Result.Nodes.getNodeAs("zero")) {
+  const auto *Str1 = Result.Nodes.getNodeAs("str1");
+  const auto *Str2 = Result.Nodes.getNodeAs("str2");
+  const auto *Compare = Result.Nodes.getNodeAs("compare");
+
+  auto Diag = diag(Matched->getLocStart(), CompareMessage);
+
+  if (Str1->isArrow())
+Diag << FixItHint::CreateInsertion(Str1->getLocStart(), "*");
+
+  Diag << tooling::fixit::createReplacement(*Zero, *Str2, Ctx)
+   << tooling::fixit::createReplacement(*Compare, *Str1->getBase(),
+Ctx);
+}
+  }
+
+  // FIXME: Add fixit to fix the code for case one and two (match1).
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
===
--- clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
+++ clang-tools-extra/trunk/clang-tidy/misc/StringCompareCheck.h
@@ -0,0 +1,36 @@
+//===--- StringCompareCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_STRING_COMPARE_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// This 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 82521.
madsravn added a comment.

Changes based on comments. 
Shortened the ast matcher.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,119 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string string;
+}
+
+void func(bool b);
+
+std::string comp() {
+  std::string str("a", 1);
+  return str;
+}
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == str2) {
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 != str2) {
+  if (str1.compare("foo") == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == "foo") {
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == str1) {
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 != str1) {
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2.empty() || str1 != str2) {
+  std::string *str3 = 
+  if (str3->compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str3->compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (*str3 == str2) {
+  if (str2.compare(*str3) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == *str3) {
+  if (comp().compare(str1) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (comp() == str1) {
+  if (str1.compare(comp()) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == comp()) {
+  if (str1.compare(comp())) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(str2) < 0) {
+  }
+  if (str1.compare(str2) == 2) {
+  }
+  if (str1.compare(str2) == -3) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == -1) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn added inline comments.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:37
+  hasArgument(0, expr().bind("str2")), argumentCountIs(1),
+  callee(memberExpr(has(implicitCastExpr(anyOf(
+  
has(callExpr(has(implicitCastExpr(has(declRefExpr().bind("str1")),

malcolm.parsons wrote:
> Do you really care what the callee expression is? 
> Use `isArrow()` on the `MemberExpr` to check if it's a pointer.
How else would I get str1? Using the below snippet, I only get str1.compare and 
str1->compare instead of str1. 
Given a MemberExpr (str1.compare) is there an easy way to extract str1? 

```
const auto StrCompare = cxxMemberCallExpr(
  callee(cxxMethodDecl(hasName("compare"),
   ofClass(classTemplateSpecializationDecl(
   hasName("::std::basic_string"),
  hasArgument(0, expr().bind("str2")), argumentCountIs(1),
  callee(memberExpr().bind("str1")))
```


https://reviews.llvm.org/D27210



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

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 82518.
madsravn added a comment.

Reviews based on comments. Removed check for suspicious string compare.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,119 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string string;
+}
+
+void func(bool b);
+
+std::string comp() {
+  std::string str("a", 1);
+  return str;
+}
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == str2) {
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 != str2) {
+  if (str1.compare("foo") == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == "foo") {
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == str1) {
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 != str1) {
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2.empty() || str1 != str2) {
+  std::string *str3 = 
+  if (str3->compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str3->compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (*str3 == str2) {
+  if (str2.compare(*str3) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == *str3) {
+  if (comp().compare(str1) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (comp() == str1) {
+  if (str1.compare(comp()) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == comp()) {
+  if (str1.compare(comp())) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(str2) < 0) {
+  }
+  if (str1.compare(str2) == 2) {
+  }
+  if (str1.compare(str2) == -3) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == -1) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

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



Comment at: clang-tidy/misc/MiscStringCompareCheck.h:24
+/// 
http://clang.llvm.org/extra/clang-tidy/checks/misc-string-compare-check.html
+class MiscStringCompareCheck : public ClangTidyCheck {
+public:

malcolm.parsons wrote:
> Remove `Misc`.
> 
> Did you use add_new_check.py to add this check?
No, but the files I was looking at had the same naming convention. Maybe 
something has changed in that regards recently? 

I will fix it.



Comment at: clang-tidy/misc/StringCompareCheck.cpp:25
+"operator instead";
+static const StringRef GuaranteeMessage = "'compare' is not guaranteed to "
+  "return -1 or 1; check for bigger or 
"

malcolm.parsons wrote:
> misc-suspicious-string-compare warns about suspicious `strcmp()`; maybe it 
> should handle `string::compare()` too.
Do you suggest that I move this check to misc-suspicious-string-compare? Or 
should we just remove it from here? 



Comment at: docs/clang-tidy/checks/misc-string-compare.rst:10
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the 
lexicographical 
+relationship between the strings compared. If an equality or inequality check

xazax.hun wrote:
> As far as I remember this is not true. The  ``compare`` method can return any 
> integer number, only the sign is defined. It is not guaranteed to return -1 
> or 1 in case of inequality.
This is true. I checked it - it is just some implementations which tend to use 
-1, 0 and 1. However, the specification says negative, 0 and positive. I will 
correct it. Thanks



Comment at: test/clang-tidy/misc-string-compare.cpp:9
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test 
equality of strings; use the string equality operator instead 
[misc-string-compare]

malcolm.parsons wrote:
> Some other test ideas:
> 
> ```
> if (str1.compare("foo")) {}
> 
> return str1.compare(str2) == 0;
> 
> func(str1.compare(str2) != 0);
> 
> if (str2.empty() || str1.compare(str2) != 0) {}
> ```
None of those fit the ast match. 

I think it's fine as it is now. If the matcher will be expanded to check for 
some of those cases, I think more test cases are needed.


https://reviews.llvm.org/D27210



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

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 82513.
madsravn added a comment.

Updated according to comments.

I have decided to keep the fixit for match1 a FIXME.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string string;
+}
+
+void func(bool b);
+
+std::string comp() {
+  std::string str("a", 1);
+  return str;
+}
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == str2) {
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 != str2) {
+  if (str1.compare("foo") == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == "foo") {
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == str1) {
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 != str1) {
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2.empty() || str1 != str2) {
+  std::string *str3 = 
+  if (str3->compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str3->compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (*str3 == str2) {
+  if (str2.compare(*str3) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == *str3) {
+  if (comp().compare(str1) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (comp() == str1) {
+  if (str1.compare(comp()) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == comp()) {
+  if (str1.compare(comp())) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str1.compare(str2) == 1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1; check for bigger or smaller than 0 instead
+  if (str1.compare(str2) == -1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(str2) < 0) {
+  }
+  if (str1.compare(str2) == 2) {
+  }
+  if (str1.compare(str2) == -3) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 81885.
madsravn added a comment.

Small changes made by suggestions. strCompare is now with uppercase: StrCompare

Checking for str.compare(str) == {-1,1} as well.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,121 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string string;
+}
+
+void func(bool b);
+
+std::string comp() {
+  std::string str("a", 1);
+  return str;
+}
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == str2) {
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 != str2) {
+  if (str1.compare("foo") == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == "foo") {
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == str1) {
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 != str1) {
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2.empty() || str1 != str2) {
+  std::string *str3 = 
+  if (str3->compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str3->compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (*str3 == str2) {
+  if (str2.compare(*str3) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == *str3) {
+  if (comp().compare(str1) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (comp() == str1) {
+  if (str1.compare(comp()) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == comp()) {
+  if (str1.compare(comp())) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str1.compare(str2) == 1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1; check for bigger or smaller than 0 instead
+  if (str1.compare(str2) == -1) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: 'compare' is not guaranteed to return -1 or 1;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(str2) < 0) {
+  }
+  if (str1.compare(str2) == 2) {
+  }
+  if (str1.compare(str2) == -3) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 81610.
madsravn added a comment.

Updated the matcher to find suggested occurences.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,111 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+bool operator==(const basic_string , const char *);
+typedef basic_string string;
+}
+
+void func(bool b);
+
+std::string comp() {
+  std::string str("a", 1);
+  return str;
+}
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == str2) {
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 != str2) {
+  if (str1.compare("foo") == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == "foo") {
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == str1) {
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 != str1) {
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2.empty() || str1 != str2) {
+  std::string *str3 = 
+  if (str3->compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  if (str3->compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (*str3 == str2) {
+  if (str2.compare(*str3) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == *str3) {
+  if (comp().compare(str1) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (comp() == str1) {
+  if (str1.compare(comp()) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == comp()) {
+  if (str1.compare(comp())) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 81260.
madsravn added a comment.

Updated matcher.

Made fixits for some cases

Updated the documentation.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,82 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+class basic_string {
+public:
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string ) const;
+  int compare(const C *) const;
+  int compare(int, int, const basic_string ) const;
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use 'compare' to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == str2) {
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 != str2) {
+  if (str1.compare("foo") == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str1 == "foo") {
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 == str1) {
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2 != str1) {
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use 'compare' to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use 'compare' to test equality of strings;
+  // CHECK-FIXES: if (str2.empty() || str1 != str2) {
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+  if (str1.compare(1, 3, str2)) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,54 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns a negative number, a positive number or 
+zero depending on the lexicographical relationship between the strings compared. 
+If an equality or inequality check can suffice, that is recommended. This is 
+recommended to avoid the risk of incorrect interpretation of the return value
+and to simplify the code. The string equality and inequality operators can
+also be faster than the ``compare`` method due to early termination.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (!str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (str1.compare(str2) == 0) {
+  }
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2) != 0) {
+  }
+
+  // use str1 == str2 instead.
+  if (0 == str1.compare(str2)) {
+  }
+
+  // use str1 != str2 instead.
+  if (0 != str1.compare(str2)) {
+  }
+
+  // Use str1 == "foo" instead.
+  if (str1.compare("foo") == 0) {
+  }
+
+The above code examples shows the 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 80177.
madsravn added a comment.

Did as comments suggested: Fixed the description about compare returning -1, 0 
or 1. Fixed the ast matcher to only find compare with one argument. 
Clang-formatted everything. Added a new test (str.compare("foo")) and wrote a 
FIXME for the fixit.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,72 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+struct basic_string {
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string );
+  int compare(const C *);
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (str1.compare("foo")) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use compare to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use compare to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,47 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns a negative number, a positive number or 
+zero depending on the lexicographical relationship between the strings compared. 
+If an equality or inequality check can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (!str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (str1.compare(str2) == 0) {
+  }
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2) != 0) {
+  }
+
+  // use str1 == str2 instead.
+  if (0 == str1.compare(str2)) {
+  }
+
+  // use str1 != str2 instead.
+  if (0 != str1.compare(str2)) {
+  }
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79961.
madsravn added a comment.

Fixed broken tests.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+struct basic_string {
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string );
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:8: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:7: warning: do not use compare to test equality of strings;
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use compare to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-2]]:23: warning: do not use compare to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,47 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (!str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (str1.compare(str2) == 0) {
+  }
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2) != 0) {
+  }
+
+  // use str1 == str2 instead.
+  if (0 == str1.compare(str2)) {
+  }
+
+  // use str1 != str2 instead.
+  if (0 != str1.compare(str2)) {
+  }
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79958.
madsravn marked 5 inline comments as done.
madsravn added a comment.

Updated according to comments. Still missing fixit.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,68 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator>
+struct basic_string {
+  basic_string();
+  basic_string(const C *, unsigned int size);
+  int compare(const basic_string );
+  bool empty();
+};
+bool operator==(const basic_string , const basic_string );
+bool operator!=(const basic_string , const basic_string );
+typedef basic_string string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if (str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if (!str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) == 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (0 == str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  if (0 != str1.compare(str2)) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings;
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use compare to test equality of strings;
+  if (str2.empty() || str1.compare(str2) != 0) {
+  }
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use compare to test equality of strings;
+}
+
+void Valid() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+  if (str1 == str2) {
+  }
+  if (str1 != str2) {
+  }
+  if (str1.compare(str2) == 1) {
+  }
+  if (str1.compare(str2) == str1.compare(str2)) {
+  }
+  if (0 == 0) {
+  }
+  if (1 == str1.compare(str2)) {
+  }
+  if (str1.compare(str2) > 0) {
+  }
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,47 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (!str1.compare(str2)) {
+  }
+
+  // use str1 == str2 instead.
+  if (str1.compare(str2) == 0) {
+  }
+
+  // use str1 != str2 instead.
+  if (str1.compare(str2) != 0) {
+  }
+
+  // use str1 == str2 instead.
+  if (0 == str1.compare(str2)) {
+  }
+
+  // use str1 != str2 instead.
+  if (0 != str1.compare(str2)) {
+  }
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79788.
madsravn added a comment.

Trimmed down the ast matcher a little.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,56 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator >
+struct basic_string {
+  basic_string();
+  basic_string(const C*, unsigned int size);
+  int compare(const basic_string& str);
+  bool empty();
+};
+  bool operator==(const basic_string lhs, const basic_string rhs);
+  bool operator!=(const basic_string lhs, const basic_string rhs);
+typedef basic_string string;
+}
+
+void func(bool b);
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(!str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:7: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) == 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) != 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:6: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  func(str1.compare(str2));
+  // CHECK-MESSAGES: [[@LINE-1]]:8: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str2.empty() || str1.compare(str2) != 0) { }
+  // CHECK-MESSAGES: [[@LINE-1]]:22: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+
+}
+
+void Valid() {
+std::string str1("a", 1);
+std::string str2("b", 1);
+if(str1 == str2) {}
+if(str1 != str2) {}
+if(str1.compare(str2) == 1) {}
+if(str1.compare(str2) == str1.compare(str2)) {}
+if(0 == 0) {}
+if(1 == str1.compare(str2)) {}
+if(str1.compare(str2) > 0) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  if(str1.compare(str2)) {} // use str1 != str2 instead
+  
+  if(!str1.compare(str2)) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) == 0) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) != 0) {} // use str1 != str2 instead
+
+  if(0 == str1.compare(str2)) {} // use str1 == str2 instead
+
+  if(0 != str1.compare(str2)) {} // use str1 != str2 instead
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
Index: 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79747.
madsravn marked 2 inline comments as done.
madsravn added a comment.

Added integerLiteral on both sides of the == operator.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,49 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator >
+struct basic_string {
+  basic_string();
+  basic_string(const C*, unsigned int size);
+  int compare(const basic_string& str);
+};
+  bool operator==(const basic_string lhs, const basic_string rhs);
+  bool operator!=(const basic_string lhs, const basic_string rhs);
+typedef basic_string string;
+}
+
+
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(!str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) == 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) != 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+}
+
+void Valid() {
+std::string str1("a", 1);
+std::string str2("b", 1);
+if(str1 == str2) {}
+if(str1 != str2) {}
+if(str1.compare(str2) == 1) {}
+if(str1.compare(str2) == str1.compare(str2)) {}
+if(0 == 0) {}
+if(1 == str1.compare(str2)) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  if(str1.compare(str2)) {} // use str1 != str2 instead
+  
+  if(!str1.compare(str2)) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) == 0) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) != 0) {} // use str1 != str2 instead
+
+  if(0 == str1.compare(str2)) {} // use str1 == str2 instead
+
+  if(0 != str1.compare(str2)) {} // use str1 != str2 instead
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+
 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79729.
madsravn marked 3 inline comments as done.
madsravn added a comment.

Diff reflecting changes suggested by comments.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,45 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+namespace std {
+template 
+class allocator {};
+template 
+class char_traits {};
+template , typename A = std::allocator >
+struct basic_string {
+  basic_string();
+  basic_string(const C*, unsigned int size);
+  int compare(const basic_string& str);
+};
+  bool operator==(const basic_string lhs, const basic_string rhs);
+  bool operator!=(const basic_string lhs, const basic_string rhs);
+typedef basic_string string;
+}
+
+
+
+void Test() {
+  std::string str1("a", 1);
+  std::string str2("b", 1);
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(!str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) == 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) != 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+}
+
+void Valid() {
+std::string str1("a", 1);
+std::string str2("b", 1);
+if(str1 == str2) {}
+if(str1 != str2) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  if(str1.compare(str2)) {} // use str1 != str2 instead
+  
+  if(!str1.compare(str2)) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) == 0) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) != 0) {} // use str1 != str2 instead
+
+  if(0 == str1.compare(str2)) {} // use str1 == str2 instead
+
+  if(0 != str1.compare(str2)) {} // use str1 != str2 instead
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+
 - New `misc-use-after-move
   `_ check
 
Index: 

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn updated this revision to Diff 79624.
madsravn added a comment.

Updated per comments


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscTidyModule.cpp
  clang-tidy/misc/StringCompareCheck.cpp
  clang-tidy/misc/StringCompareCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,29 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+#include 
+
+void Test() {
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(!str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) == 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) != 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 == str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(0 != str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+
+}
+
+void Valid() {
+std::string str1{"a"};
+std::string str2{"b"};
+if(str1 == str2) {}
+if(str1 != str2) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's ``compare`` method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns ``-1``, ``0`` or ``1`` depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  if(str1.compare(str2)) {} // use str1 != str2 instead
+  
+  if(!str1.compare(str2)) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) == 0) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) != 0) {} // use str1 != str2 instead
+
+  if(0 == str1.compare(str2)) {} // use str1 == str2 instead
+
+  if(0 != str1.compare(str2)) {} // use str1 != str2 instead
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses ``compare`` to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -75,6 +75,11 @@
 
 - `misc-pointer-and-integral-operation` check was removed.
 
+- New `misc-string-compare
+  `_ check
+
+  Warns about using ``compare`` to test for string equality or ineqaulity.
+
 - New `misc-use-after-move
   `_ check
 
Index: clang-tidy/misc/StringCompareCheck.h
===
--- clang-tidy/misc/StringCompareCheck.h
+++ clang-tidy/misc/StringCompareCheck.h
@@ -0,0 +1,36 @@
+//===--- StringCompareCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//

[PATCH] D27210: [clang-tidy] misc-string-compare. Adding a new check to clang-tidy

[Mads Ravn via Phabricator via cfe-commits]

madsravn removed rL LLVM as the repository for this revision.
madsravn updated this revision to Diff 79610.
madsravn added a comment.

Updated the patch to include changes suggested by comments.


https://reviews.llvm.org/D27210

Files:
  clang-tidy/misc/CMakeLists.txt
  clang-tidy/misc/MiscStringCompareCheck.cpp
  clang-tidy/misc/MiscStringCompareCheck.h
  clang-tidy/misc/MiscTidyModule.cpp
  docs/clang-tidy/checks/list.rst
  docs/clang-tidy/checks/misc-string-compare.rst
  test/clang-tidy/misc-string-compare.cpp

Index: test/clang-tidy/misc-string-compare.cpp
===
--- test/clang-tidy/misc-string-compare.cpp
+++ test/clang-tidy/misc-string-compare.cpp
@@ -0,0 +1,24 @@
+// RUN: %check_clang_tidy %s misc-string-compare %t -- -- -std=c++11
+
+#include 
+
+void Test() {
+  std::string str1{"a"};
+  std::string str2{"b"};
+
+  if(str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(!str1.compare(str2)) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) == 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+  if(str1.compare(str2) != 0) {}
+  // CHECK-MESSAGES: [[@LINE-1]]:3: warning: do not use compare to test equality of strings; use the string equality operator instead [misc-string-compare]
+}
+
+void Valid() {
+std::string str1{"a"};
+std::string str2{"b"};
+if(str1 == str2) {}
+if(str1 != str2) {}
+}
Index: docs/clang-tidy/checks/misc-string-compare.rst
===
--- docs/clang-tidy/checks/misc-string-compare.rst
+++ docs/clang-tidy/checks/misc-string-compare.rst
@@ -0,0 +1,31 @@
+.. title:: clang-tidy - misc-string-compare
+
+misc-string-compare
+===
+
+Finds string comparisons using the compare method.
+
+A common mistake is to use the string's compare method instead of using the 
+equality or inequality operators. The compare method is intended for sorting
+functions and thus returns -1, 0 or 1 depending on the lexicographical 
+relationship between the strings compared. If an equality or inequality check
+can suffice, that is recommended.
+
+Examples:
+
+.. code-block:: c++
+
+  std::string str1{"a"};
+  std::string str2{"b"};
+  
+  if(str1.compare(str2)) {} // use str1 != str2 instead
+  
+  if(!str1.compare(str2)) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) == 0) {} // use str1 == str2 instead
+  
+  if(str1.compare(str2) != 0) {} // use str1 != str2 instead
+
+The above code examples shows the list of if-statements that this check will
+give a warning for. All of them uses compare to check if equality or 
+inequality of two strings instead of using the correct operators.
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -80,6 +80,7 @@
misc-sizeof-container
misc-sizeof-expression
misc-static-assert
+   misc-string-compare
misc-string-constructor
misc-string-integer-assignment
misc-string-literal-with-embedded-nul
Index: clang-tidy/misc/MiscTidyModule.cpp
===
--- clang-tidy/misc/MiscTidyModule.cpp
+++ clang-tidy/misc/MiscTidyModule.cpp
@@ -22,6 +22,7 @@
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
+#include "MiscStringCompareCheck.h"
 #include "MisplacedConstCheck.h"
 #include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
@@ -105,6 +106,7 @@
 CheckFactories.registerCheck(
 "misc-sizeof-expression");
 CheckFactories.registerCheck("misc-static-assert");
+CheckFactories.registerCheck("misc-string-compare");
 CheckFactories.registerCheck(
 "misc-string-constructor");
 CheckFactories.registerCheck(
Index: clang-tidy/misc/MiscStringCompareCheck.h
===
--- clang-tidy/misc/MiscStringCompareCheck.h
+++ clang-tidy/misc/MiscStringCompareCheck.h
@@ -0,0 +1,36 @@
+//===--- MiscStringCompareCheck.h - clang-tidy---*- C++ -*-===//
+//
+// The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===--===//
+
+#ifndef MISC_STRING_COMPARE_CHECK_H
+#define MISC_STRING_COMPARE_CHECK_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace