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

[Aaron Ballman via cfe-commits]

aaron.ballman added a comment.

In https://reviews.llvm.org/D22725#505020, @JDevlieghere wrote:

> Addresses comments from Aaron Ballman
>
> @aaron.ballman Thanks for the thorough review! Can you check whether the tests I added address your concerns? Could you also elaborate on the case with the C-function pointer? Unless I explicitly cast it to void* the compiler rejects will reject it as an argument to memcpy. Am I missing a case where this could go wrong? I still added it to the pointer arithmetic check though, just to be sure.


The tests added mostly cover them -- I elaborated on the function pointer case, which I don't *think* will go wrong with this check, but should have a pathological test case for just to be sure.


================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:26
@@ +25,3 @@
+static QualType getStrippedType(QualType T) {
+  while (const auto *PtrType = T->getAs<PointerType>())
+    T = PtrType->getPointeeType();
----------------
Consider a pathological case like:
```
#include <string.h>

void f();

int main() {
  typedef void (__cdecl *fp)();
  
  fp f1 = f;
  fp f2;
  memcpy(&f2, &f1, sizeof(fp)); // transforms into std::copy(&f1, &f1, &f2); ?
}
```
The calling convention is important in the test because `getStrippedType()` isn't looking through attributed types, which I *think* that declaration would rely on. I don't think it will cause problems in this case, but the test will ensure it.

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:53-54
@@ +52,4 @@
+                                  const StringRef Arg2) {
+  Twine Replacement = Function + "(" + Arg0 + ", " + Arg1 + ", " + Arg2 + ")";
+  return Replacement.str();
+}
----------------
A more idiomatic way to write this is: `return (Twine(Function) + "(" + Arg0...<etc>).str();`

================
Comment at: clang-tidy/modernize/UseAlgorithmCheck.cpp:106
@@ +105,3 @@
+  const SourceLocation Loc = MatchedExpr->getExprLoc();
+  auto Diag = diag(Loc, "'" + MatchedName.str() +
+                            "' reduces type-safety, consider using '" +
----------------
Instead of manually composing the diagnostic, it should use placeholders. e.g.,
```
diag(Loc, "%0 reduces type-safety, consider using '%1' instead) << Callee << ReplacedName;
```
(This makes it easier if we ever decide to localize our diagnostics.) Note that there's no quoting required around %0 because we're passing in a NamedDecl argument and the diagnostics engine understands how to display that.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725