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
