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
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
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.
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
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 =
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:
> > >
> > >
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:
> > >
>
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:
> > >
>
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
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
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
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
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
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
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
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
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
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 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 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 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 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 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
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
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
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.
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
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
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
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
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
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 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 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 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 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 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
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:
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:
39 matches
Mail list logo
