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<CallExpr>("match");
+ SourceManager &SM = *Result.SourceManager;
+ LangOptions LangOpts = getLangOpts();
----------------
No need for this to be a separate variable.
================
Comment at: clang-tidy/modernize/ReplaceRandomShuffleCheck.cpp:72
+ SourceManager &SM = *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
