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