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

[Alexander Kornienko via cfe-commits]

alexfh 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"}}) {
----------------
aaron.ballman wrote:
> alexfh wrote:
> > I'm not sure this works on MSVC2013. Could someone try this before submitting?
> It does not work in MSVC 2013.
Thanks for verifying. Then it should be just

  Replacements["memcpy"] = "std::copy";
  Replacements["memset"] = "std::fill";

Maybe even remove the map completely and just hardcode the two options in `UseAlgorithmCheck::check`. Or are we expecting other similar functions to be added to the list? (I guess, unlikely)

================
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;
----------------
aaron.ballman wrote:
> 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.
It's a terminology issue, I think. Clang-tidy uses clang "warnings" for all kinds of warnings, issues, suggestions or recommendations it reports. If we want to extend the range while remaining in the clang  diagnostic engine bounds, we could use Remark for some diagnostics, but I think, the mapping would still need to be configurable by the user. Alternatively, we could introduce some kind of a more granular integer-based severity level. However, I don't have an immediate use case for either of these, so I'd leave design to someone, who actually needs this and can explain the motivation.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725