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