[PATCH] D22725: [clang-tidy] Add check 'modernize-use-algorithm'
Aaron Ballman via cfe-commits
cfe-commits at lists.llvm.org
Thu Aug 4 06:21:31 PDT 2016
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
More information about the cfe-commits
mailing list