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

[Aaron Ballman via cfe-commits]

aaron.ballman added inline comments.

================
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"}}) {
----------------
alexfh wrote:
> I'm not sure this works on MSVC2013. Could someone try this before submitting?
It does not work in MSVC 2013.

================
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;
----------------
alexfh wrote:
> 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.
Drat. I am not keen on this being a warning (let alone an error) because it really doesn't warn the user against anything bad (the type safety argument is tenuous at best), and it's arguable whether this actually modernizes code. Do you think there's sufficient utility to this check to warrant it being default-enabled as part of the modernize suite? For instance, we have 368 instances of memcpy() and 171 instances of std::copy() in the LLVM code base, which is an example of a very modern C++ code base. I'm not opposed to the check, just worried that it will drown out diagnostics that have more impact to the user.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725