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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
