[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/UseAlgorithmCheck.cpp:37
@@ +36,3 @@
+static bool areTypesCompatible(QualType Left, QualType Right) {
+  return getStrippedType(Left) == getStrippedType(Right);
+}
----------------
Please inline this function as well. It's only used once and the implementation + the comment that is already in the call site are more than enough to explain the intent.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:41
@@ +40,3 @@
+static bool noPointerArithmetic(QualType T) {
+  return T->isVoidPointerType() || T->isFunctionPointerType();
+}
----------------
I'd probably inline this function as well. Its name is not particularly clear anyway (could be better with 'doesNotSupportPointerArithmetic` or similar, but I think, inlining it is even better).

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:48
@@ +47,3 @@
+          Options.get("IncludeStyle", "llvm"))) {
+  Replacements["memcpy"] = "std::copy";
+  Replacements["memset"] = "std::fill";
----------------
Actually, I don't think we need a map of exactly two entries, since the order of arguments still has to be hardcoded. Let's just do a couple of `if`s or a `StringSwitch` in the `check()` method.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:141
@@ +140,3 @@
+          Result.SourceManager->getFileID(Loc), "algorithm",
+          /*IsAngled=*/true)) {
+    Diag << *IncludeFixit;
----------------
No braces around single-statement `if` bodies, please.

================
Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:51
@@ +50,3 @@
+
+    std::copy(foo ? bar : baz, foo ? baz : baz + size, dest);
+
----------------
I think, we need to fix this right away instead of documenting this bug. Two things here:

  1. problems related to the operator precedence can be fixed by adding parentheses, when needed (see how a similar issue was solved in `SimplifyBooleanExprCheck`; here we can make a `needsParensBeforePlus` function similar to `needsParensAfterUnaryNegation`).
  2. the check should avoid replacements that duplicate an expression with side effects that can result from increment/decrement operators, assignments and some function calls. Function calls is the most difficult part here, since not all function calls have side effects (or are expensive). One approach here could be to add a `const auto &` variable to keep the value of the argument we need to repeat, if it's anything more complex than just a DeclRefExpr.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725