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

[Aaron Ballman via cfe-commits]

aaron.ballman requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:25
@@ +24,3 @@
+
+static QualType getStrippedType(QualType T) {
+  while (const auto *PtrType = T->getAs<PointerType>())
----------------
I'd like to see some test cases involving attributed types, typedefs to pointers and non-pointers, and multidimensional arrays. For instance, I'm curious how well this handles `typedef int some_type; typedef some_type * const * volatile *foo_ptr;` or `typedef void (__cdecl fn_ptr)(int);`

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:49-50
@@ +48,4 @@
+                                  const StringRef Arg2) {
+  return Function.str() + "(" + Arg0.str() + ", " + Arg1.str() + ", " +
+         Arg2.str() + ")";
+}
----------------
Please use Twine for this sort of thing.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:65
@@ +64,3 @@
+void UseAlgorithmCheck::registerMatchers(MatchFinder *Finder) {
+  // Only register the matchers for C++
+  if (!getLangOpts().CPlusPlus)
----------------
Missing full stop.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:93
@@ +92,3 @@
+
+  const auto MatchedName = Callee->getNameAsString();
+
----------------
Please do not use auto here as the type is not spelled out in the RHS (same elsewhere).

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:96
@@ +95,3 @@
+  // Check if matched name is in map of replacements.
+  const auto it = Replacements.find(MatchedName);
+  assert(it != Replacements.end());
----------------
You should ensure that the callee is actually the function we care about. Consider:
```
namespace awful {
void memcpy(int, int, int);
}
using namespace awful;
```
Such a use of memcpy() would trigger your check and create an invalid replacement, I think (and there should be a test for this).


================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:97
@@ +96,3 @@
+  const auto it = Replacements.find(MatchedName);
+  assert(it != Replacements.end());
+  const auto ReplacedName = it->second;
----------------
Please add some "help text" to the assert in case it ever triggers. e.g., && "Replacements list does not match list of registered matcher names" or some such.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:101
@@ +100,3 @@
+  const auto Loc = MatchedExpr->getExprLoc();
+  auto Diag = diag(Loc, "Use " + ReplacedName + " instead of " + MatchedName);
+
----------------
Diagnostics should start with a lowercase letter.

More importantly, this warning doesn't actually describe a problem to the user. Given the fact that this only should be triggered on uses where memcpy and std::copy are interchangeable, it's not really a *warning* at all (due to the interchangeability) as there is nothing dangerous about the original code that the suggestion will fix. @alexfh, what do you think of the idea of this being a Note rather than a Warning? I know it's unorthodox, but literally every instance of this diagnostic can be ignored or replaced and there should be no semantic effect on the code either way. Most other checks in modernize have more obvious benefits to modifying the code and so Warning is reasonably appropriate.

If this text remains a warning, it should describe what is dangerous with the code, not simply how to transform the code. Perhaps "'memcpy' reduces type-safety, consider using 'std::copy' instead" or something along those lines?

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:107-108
@@ +106,4 @@
+
+  const auto Arg0Type = MatchedExpr->getArg(0)->IgnoreImpCasts()->getType();
+  const auto Arg1Type = MatchedExpr->getArg(1)->IgnoreImpCasts()->getType();
+
----------------
What about parens? e.g., memcpy((foo + 12), (bar), (a + b - 10));

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:118
@@ +117,3 @@
+
+  // Cannot do arithmetic on void pointer.
+  if (getStrippedType(Arg0Type)->isVoidType() ||
----------------
Same for function pointer types.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:137
@@ +136,3 @@
+  } else {
+    // Rearrangement of arguments for memcpy
+    // (dest, src, count) becomes (src, src + count, dest).
----------------
Missing a colon at the end of this line.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.h:1
@@ +1,2 @@
+//===--- UseAlgorithmCheck.h - clang-tidy-----------------------------*- C++ -*-===//
+//
----------------
Line length does not match the closing ASCII art line length.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.h:20
@@ +19,3 @@
+
+/// Replaces memcpy with std::copy
+///
----------------
This is no longer accurate, and is also missing the full stop.

================
Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:6
@@ +5,3 @@
+
+Replaces calls to ``memcpy`` and ``memset`` with ``std::copy``, and
+``std::fill`` respectively. The advantages of using these algorithm functions
----------------
Spurious comma. 

================
Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:7
@@ +6,3 @@
+Replaces calls to ``memcpy`` and ``memset`` with ``std::copy``, and
+``std::fill`` respectively. The advantages of using these algorithm functions
+is that they are at least as efficient, more general and type-aware.
----------------
Comma before "respectively".

================
Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:8
@@ +7,3 @@
+``std::fill`` respectively. The advantages of using these algorithm functions
+is that they are at least as efficient, more general and type-aware.
+
----------------
Comma before "and" because Oxford commas are the best commas.

================
Comment at: docs/clang-tidy/checks/modernize-use-algorithm.rst:10-11
@@ +9,4 @@
+
+Furthermore, by using the algorithms the types remain intact as opposed to
+being discarded by the C-style functions. This allows the implementation to
+make use use of type information to further optimize. One example of such
----------------
Remove the "Furthermore," as this is a continuation of the "type-aware" statement above?

================
Comment at: test/clang-tidy/modernize-use-algorithm.cpp:2
@@ +1,3 @@
+// RUN: %check_clang_tidy %s modernize-use-algorithm %t
+
+// CHECK-FIXES: #include <algorithm>
----------------
Remove blank line.

Missing tests for unqualified uses of calling memcpy() and memset().

================
Comment at: test/clang-tidy/modernize-use-algorithm.cpp:26
@@ +25,3 @@
+
+  void* baz = bar;
+  std::memcpy(baz, foo, sizeof bar);
----------------
The asterisk binds to the identifier -- should run clang-format over the file.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725