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

[Gábor Horváth via Phabricator via cfe-commits]

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