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

[Piotr Padlewski via cfe-commits]

Prazek added a subscriber: Prazek.
Prazek added a comment.

Thanks for the contribution. Is it your first check?

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.

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)

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.

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!


================
Comment at: clang-tidy/misc/ReplaceMemcpyCheck.cpp:25
@@ +24,3 @@
+void ReplaceMemcpyCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("memcpy")))).bind("expr"), this);
----------------
add if here to discard the non c++ calls. (the same as you see in most of the checks)

================
Comment at: clang-tidy/misc/ReplaceMemcpyCheck.cpp:26
@@ +25,3 @@
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("memcpy")))).bind("expr"), this);
+}
----------------
you can check argument count here.

================
Comment at: clang-tidy/misc/ReplaceMemcpyCheck.cpp:40-41
@@ +39,4 @@
+
+  if (MatchedExpr->getNumArgs() != 3)
+    return;
+
----------------
so this can be checked in matcher 

================
Comment at: clang-tidy/misc/ReplaceMemcpyCheck.cpp:55
@@ +54,3 @@
+  diag(MatchedExpr->getExprLoc(), "Use std::copy instead of memcyp")
+      << FixItHint::CreateReplacement(MatchedExpr->getSourceRange(), Insertion);
+}
----------------
don't make replacement if it is in macro (use .isMacroID on the location).

Also add tests with macros.

================
Comment at: clang-tidy/misc/ReplaceMemcpyCheck.h:30
@@ +29,3 @@
+
+  static StringRef GetText(const Expr *Expression, const SourceManager &SM,
+                           const LangOptions &LangOpts);
----------------
If it is static, then move it to .cpp file.

You could also remove static and arguments that you have access from class and move it to private.

Also I think the function name convention is lowerCamelCase

================
Comment at: test/clang-tidy/misc-replace-memcpy.cpp:3-5
@@ +2,5 @@
+
+#include <cstring>
+#include <algorithm>
+#include <stdio.h>
+
----------------
don't include the stl, because it will make the test undebugable (the ast is too big)

Mock the functions that you need - std::copy and std::memcpy. you don't have to give definition, but just make sure the declaration of the functions are the same.


Also check if check will add <algorithm> here.


Repository:
  rL LLVM

https://reviews.llvm.org/D22725