JDevlieghere added a comment. In https://reviews.llvm.org/D22725#493947, @Prazek wrote:
> Thanks for the contribution. Is it your first check? Yes, it is! :-) > Some main issues: > > 1. I think it would be much better to move this check to modernize module. I > think the name 'modernize-use-copy' would follow the convention, or just > 'modernize-replace-memcpy' also works, maybe it is even better. Your choice. > Use rename_check.py to rename the check. Agreed, I moved and renamed the check. > 2. Please add include fixer that will include <algorithm> if it is not yet > included, so the code will compile after changes. Look at > DeprecatedHeadersCheck.cpp or PassByValueCheck ( or grep for > registerPPCallbacks) Done > 3. The extra parens are not ideal thing. Can you describe (with examples) in > which situations it would not compile/ something bad would happen if you > wouldn't put the parens? I think you could check it in matchers and then > apply parens or not. 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... > 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! 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
