[PATCH] D22725: [clang-tidy] Add check 'misc-replace-memcpy'

[Piotr Padlewski via cfe-commits]

Prazek added a comment.

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

> In https://reviews.llvm.org/D22725#493947, @Prazek wrote:
>
> > Thanks for the contribution. Is it your first check?
>
>
> Yes, it is! :-)


Cool! hope to see some other cool checks. You could probably use pretty much the same code, and make modernize-use-fill for replacing memset and
modernize-use-move to change memmove -> move. The other thing that I can think of, is also to detect when you could use std::copy_n istead.

Maybe the right way would be to have check called 'modernize-use-sequence-algorithm' or just 'modernize-use-algorithm' that would basically do all those stuff. It would be good to not introduce 5 new check that pretty much do the same thing and also the user would want to have them all or none.
You can look how I did it with modernize-use-make-shared, it would be pretty much the same.

> I have included an example in the documentation with the ternary conditional operator. I agree that it's not ideal, but I tried to be defensive. Basically anything with lower precedence than the + operator might cause an issue, and this is quite a lot...


So It should not be very hard. You would just have to check if the second or third argument to memcpy is one of [list here] of the things that 
would make it bad. And you can probably write easy matcher for this.
I understand that it probably will take the double of time that you have spend on this check, but this will actually is the thing here - you want your check to produce the fixes that makes sense. When you will run it on llvm code base, I guess you will see that over 90% of the cases didn't require coma. I understand that you want to make it accurate, but the fix should not also intoruce some code that is not tidy enough.
So I guess either fix it, or wait what Alex will say about it. It might make it to upstream like this, but I hope you would fix it in some time :)

Anyway good job, hope to see some more checks from you!

> > 4. Have you run the check on LLVM code base? It uses std::copy in many places. If after running with -fix all the memcpys will be gone (check if if finds anything after second run) and the code will compile and tests will not fail then it means that your check fully works!

> 

> 

> Doing this as we speak!


When you will be finished just post a diff of changes after running it on llvm on phabricator and add me as subscriber :)


================
Comment at: clang-tidy/modernize/UseCopyCheck.cpp:38
@@ +37,3 @@
+  // Match calls to memcpy with 3 arguments
+  Finder->addMatcher(callExpr(allOf(callee(functionDecl(hasName("memcpy"))),
+                                    argumentCountIs(3)))
----------------
change memcpy to ::std::memcpy, and also make a test with some memcpy that is outside of std namespace.
You don't want to change someone else code because it uses the same name :)


Repository:
  rL LLVM

https://reviews.llvm.org/D22725