[PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'

[Alexander Kornienko via cfe-commits]

alexfh requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/modernize/ModernizeTidyModule.cpp:59
@@ -57,1 +58,3 @@
         "modernize-use-bool-literals");
+    CheckFactories.registerCheck<UseAlgorithmCheck>(
+        "modernize-use-algorithm");
----------------
Entries should be sorted alphabetically.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:50
@@ +49,3 @@
+
+static std::string getReplacement(const StringRef Function,
+                                  const StringRef Arg0, const StringRef Arg1,
----------------
I don't think this function pulls its weight. With just two callers and a trivial implementation, it seems that it's better to inline it.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:59-61
@@ +58,5 @@
+      IncludeStyle(utils::IncludeSorter::parseIncludeStyle(
+          Options.get("IncludeStyle", "llvm"))) {
+
+  for (const auto &KeyValue :
+       {std::make_pair("memcpy", "std::copy"), {"memset", "std::fill"}}) {
----------------
I'm not sure this works on MSVC2013. Could someone try this before submitting?

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:86
@@ +85,3 @@
+  // benign.
+  if (getLangOpts().CPlusPlus) {
+    Inserter = llvm::make_unique<utils::IncludeInserter>(
----------------
For consistency, I'd prefer the early return style (`if (!...) return;`).

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:102
@@ +101,3 @@
+  assert(it != Replacements.end() &&
+         "Replacements list does not match list of registered matcher names");
+  const std::string ReplacedName = it->second;
----------------
Notes are treated completely differently - each note has to be attached to a warning.

Clang-tidy can only deal with two severity levels of diagnostics: "warning" and "error", but it's better to let them be controlled by the user via `-warnings-as-errors=` command-line option or the `WarningsAsErrors` configuration file option.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:103
@@ +102,3 @@
+         "Replacements list does not match list of registered matcher names");
+  const std::string ReplacedName = it->second;
+
----------------
This should be a StringRef instead. No need to allocate a string.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:131
@@ +130,3 @@
+
+  const StringRef Arg0Text = getText(MatchedExpr->getArg(0), SM, LangOptions);
+  const StringRef Arg1Text = getText(MatchedExpr->getArg(1), SM, LangOptions);
----------------
Should `clang::tooling::fixit::getText` be used instead?

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.h:20
@@ +19,3 @@
+
+/// Replaces std::memcpy and std::memset with std::copy and std::fill,
+/// respectively.
----------------
Please enclose inline code snippets in backquotes.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725