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