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

Jonas Devlieghere via cfe-commits cfe-commits at lists.llvm.org
Sat Jul 23 12:10:37 PDT 2016


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





More information about the cfe-commits mailing list