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

Jonas Devlieghere via cfe-commits cfe-commits at lists.llvm.org
Sun Jul 24 08:19:53 PDT 2016


JDevlieghere added a comment.

In https://reviews.llvm.org/D22725#494074, @Prazek wrote:

> 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 kept everything in modernize-use-algorithm as I agree this isn't worth 3 different checks.

> 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 :)


You are right. I didn't completely finish the first run on LLVM, but none of the occurrences it had found so far would have caused an issue. I guess that means that omitting the extra parens is a sensible default for now. I will look into excluding problematic situations once I am sure this check is fully functional.

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


Thanks, I really appreciate the feedback from everyone! :)

> > > 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 :)


It's running again with the changes from the last revision included. It is taking a very long time though but I guess that's my fault for not building in release mode. I'll leave it running for now as I don't want to start over again.


https://reviews.llvm.org/D22725





More information about the cfe-commits mailing list