[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.
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.
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.
aaron.ballman added a comment. This continues to LGTM; do you need someone to land the patch for you? Comment at: test/clang-tidy/modernize-replace-random-shuffle.cpp:40 + + + std::shuffle(vec.begin(), vec.end()); Can remove the extra newline. 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.
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
[PATCH] D30158: [clang-tidy] modernize: Find usage of random_shuffle and replace it with shuffle.
aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
There are a few small nits I've mentioned, but otherwise LGTM.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:34-35
+
+ const auto first = hasArgument(0, expr().bind("one"));
+ const auto second = hasArgument(1, expr().bind("two"));
+ const auto third = hasArgument(2, expr().bind("three"));
No need to bind either of these -- the bound name never gets used.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:36
+ const auto second = hasArgument(1, expr().bind("two"));
+ const auto third = hasArgument(2, expr().bind("three"));
+ Finder->addMatcher(
Can likely pick a better name to bind than "three". ;-) I would recommend
"RandomFunc" since that's what the parameter represents.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:70
+if (MatchedCallExpr->getNumArgs() == 3) {
+ auto DiagL = diag(MatchedCallExpr->getLocStart(), "'std::random_shuffle'
has been removed in C++17; use 'std::shuffle' and an alternative random
mechanism instead");
+ DiagL << FixItHint::CreateReplacement(
Wrap for 80-column (here and elsewhere).
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.
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.
jroelofs added a comment. 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. 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.
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.
jroelofs added a comment. 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. 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.
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.
aaron.ballman added a comment. 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++? 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.
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.
jroelofs added a comment. In https://reviews.llvm.org/D30158#690032, @madsravn wrote: > Looks good for the two tests the are for `random_shuffle` in llvm libc++. There were a lot more some time ago, before @mclow.lists performed this transformation on libc++'s testsuite. You might want to try this on the revision before that happened. 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.
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.
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.
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.
aaron.ballman added a comment.
Out of curiosity, have you run this over libc++ or libstdc++ test suites
involving `std::random_shuffle`? If so, were the results acceptable?
I think this generally LGTM (aside from the minor nit about naming), but I'd
like to hear from @mclow.lists where this meets his needs.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+ auto Diag = [=]() {
+std::string Message = ReplaceMessage;
madsravn wrote:
> 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?
>
>
No, this sort of expression is fine when it makes the code more simple.
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.
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.
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+ auto Diag = [=]() {
+std::string Message = ReplaceMessage;
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.
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.
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.
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+ auto Diag = [=]() {
+std::string Message = ReplaceMessage;
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()())");
}
```
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.
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.
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+ auto Diag = [=]() {
+std::string Message = ReplaceMessage;
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.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:89
+
+ std::string newname = "shuffle";
+ StringRef ContainerText = Lexer::getSourceText(
Should be `NewName` instead.
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.
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.
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.
aaron.ballman added inline comments.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:25
+static const StringRef ReplaceMessage =
+"do not use 'random_shuffle'. It is deprecated and replaced by 'shuffle'.";
+
Diagnostics are not full sentences. This should probably be:
`'std::random_shuffle' has been removed in C++17; use 'std::shuffle' instead`.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:27
+
+static const StringRef RandomFunctionMessage =
+" The old user defined 'RandomFunction' is not usable for 'shuffle'. You "
I'd feel more comfortable if both of these were `const char[]` rather than
`StringRef`, because `StringRef` doesn't typically own the underlying memory
and should not be long-lived.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:28
+static const StringRef RandomFunctionMessage =
+" The old user defined 'RandomFunction' is not usable for 'shuffle'. You "
+"need to make additional changes if you want a specific random function.";
Instead of adding additional text in this case, would it make sense to use a
different diagnostic? e.g., `'std::random_shuffle' has been removed in C++17;
use an alternative mechanism instead` (or something along those lines).
Regardless, it shouldn't be using full sentences.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:71
+ const auto *MatchedCallExpr = Result.Nodes.getNodeAs("match");
+ SourceManager = *Result.SourceManager;
+ LangOptions LangOpts = getLangOpts();
No need for this to be a separate variable.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:72
+ SourceManager = *Result.SourceManager;
+ LangOptions LangOpts = getLangOpts();
+
No need for this to be a separate variable.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:77
+
+ auto Diag = [=]() {
+std::string Message = ReplaceMessage;
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?
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:105
+
+ if (auto IncludeFixit = IncludeInserter->CreateIncludeInsertion(
+ Result.Context->getSourceManager().getFileID(
Please don't use `auto` as the type is not specified in the initialization.
Comment at: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst:8
+
+Below is two examples of what kind of occurrences will be found and two
examples of what it will be replaced with.
+
is -> are
Comment at: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst:20
+
+Both these examples will be replaced with
+
Both these -> Both of these
with -> with:
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.
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.
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.
malcolm.parsons added inline comments. Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.h:21 +/// std::random_shuffle will be removed as of C++17. This check will find and +/// replace all occurences of std::random_shuffle with std::shuffle. +/// Typo - should be occurrences. Same in 2 other places. 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.
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.
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.
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.
xazax.hun added inline comments.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:81
+ Stream << "shuffle(";
+ FirstArgument->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+ Stream << ", ";
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.
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.
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.
xazax.hun 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.";
madsravn wrote:
> 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.
I usually just make it one big line and than clang format will do a better
reflow. By default it do not remove linebreaks, even if it could improve the
layout.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:70
+ llvm::raw_string_ostream Stream(Buffer);
+ DRef->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+ StringRef StrRef(Stream.str());
madsravn wrote:
> 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 :)
I was thinking about something like getSourceText of Lexer.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:81
+ Stream << "shuffle(";
+ FirstArgument->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+ Stream << ", ";
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());
```
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:101
+
+ auto Diag = diag(MatchedCallExpr->getLocStart(), Message);
+ Diag << FixItHint::CreateRemoval(MatchedCallExpr->getSourceRange());
madsravn wrote:
> 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?
Because that might cause the code not to compile when the macro is expanded in
a different context. It is a conservative approach but other tidy checkers
usually do this as well. You can use the isMacroID() method of the source
locations.
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.
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.
xazax.hun added a comment.
Nice check! Thank you for working on this!
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.";
Maybe it would be worth to reflow this literal.
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:70
+ llvm::raw_string_ostream Stream(Buffer);
+ DRef->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+ StringRef StrRef(Stream.str());
What about accessing the buffer of the source file instead of pretty printing?
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:81
+ Stream << "shuffle(";
+ FirstArgument->printPretty(Stream, nullptr, Ctx.getPrintingPolicy());
+ Stream << ", ";
Wouldn't just using the original source range of the argument work?
What about preserving the comments next to the arguments?
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:101
+
+ auto Diag = diag(MatchedCallExpr->getLocStart(), Message);
+ Diag << FixItHint::CreateRemoval(MatchedCallExpr->getSourceRange());
You do not want to do fixits for code that is the result of macro expansions.
Comment at: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst:24
+
+ std::shuffle(vec.begin(), vec.end(), std::mt19937(std::random_device()()));
+
Isn't it a performance problem in general to always reinitialize a random
engine? Maybe the documentation should contain a notice that in case of
performance critical code the user might want to factor the last parameter out
somewhere.
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()()));
+
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?
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.
Eugene.Zelenko added inline comments. Comment at: docs/ReleaseNotes.rst:78 + + Finds and fixes usage of random_shuffle as the function has been removed from C++17. + Please add std:: and enclose random_shuffle in ``. Comment at: docs/clang-tidy/checks/modernize-replace-random-shuffle.rst:6 + +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. + I think std:: should be added. 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.
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
+
