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

[Aaron Ballman via Phabricator via cfe-commits]

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