On Sat, Jul 23, 2016 at 1:38 PM, Piotr Padlewski
<[email protected]> wrote:
> 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.
Why "modernize"? There's nothing modern about std::copy(), so I don't
see this as really relating to modernizing someone's code base.
Truth be told, I'm not entirely convinced this is a good check to have
-- I'm still wondering if there's a compelling use case where
std::copy() is an improvement over std::memcpy(). I'm guessing one
exists, but I've not seen it stated here.
~Aaron
>
> 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
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
