[PATCH] D12839: Extend MoveConstructorInitCheck to also flag constructor arguments passed by value and can be moved assigned to fields.

[Richard Smith via cfe-commits]

On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman <aaron.ballman at gmail.com>
wrote:

> aaron.ballman added reviewers: rtrieu, rsmith.
> aaron.ballman added a comment.
>
> There is a -Wpessimizing-move frontend warning that Richard added not too
> long ago, which tells the user to remove calls to std::move() because they
> pessimize the code. The new functionality you are looking to add is
> basically the opposite: it tells the user to add std::move() because the
> code is currently pessimized due to copies. Given how general that concept
> is (it doesn't just appertain to constructor initializations), I wonder if
> this makes more sense as a frontend warning like -Wpessimizing-copy.
>
> Richard (both of you), what do you think?
>

I think this is in the grey area between what's appropriate for clang-tidy
and what's appropriate as a warning, where both options have merit; the
same is true with -Wpessimizing-move. I think the difference between the
two cases is that -Wpessimizing-move detects a case where you wrote
something that doesn't do what (we think) you meant, whereas this check
detects a case where you /didn't/ write something that (we think) would
make your code better (even though both changes have similar effects, of
safely turning a copy into a move or a move into an elided move). It's a
fine line, but for me that nudges -Wpessimizing-move into a warning, and
this check into clang-tidy territory.

~Aaron
>
>
> ================
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32
> @@ +31,3 @@
> +  // excessive copying, we'll warn on them.
> +  if (Node->isDependentType()) {
> +    return false;
> ----------------
> Elide braces, here and elsewhere.
>
> ================
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36
> @@ +35,3 @@
> +  // Ignore trivially copyable types.
> +  if (Node->isScalarType() ||
> +      Node.isTriviallyCopyableType(Finder->getASTContext()) ||
> ----------------
> Can turn this into a return statement directly instead of an if.
>
> ================
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38
> @@ +37,3 @@
> +      Node.isTriviallyCopyableType(Finder->getASTContext()) ||
> +      classHasTrivialCopyAndDestroy(Node)) {
> +    return false;
> ----------------
> Why do you need classHasTrivialCopyAndDestroy() when you're already
> checking if it's a trivially copyable type?
>
> ================
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44
> @@ +43,3 @@
> +
> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,
> +                                 const CXXConstructorDecl
> &ConstructorDecl,
> ----------------
> Should return unsigned, please.
>
> ================
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:50
> @@ +49,3 @@
> +      findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam)))));
> +  auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(), Context);
> +  Occurrences += Matches.size();
> ----------------
> You don't actually need Matches, you can call match().size() instead.
>
> ================
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:52
> @@ +51,3 @@
> +  Occurrences += Matches.size();
> +  for (const auto* Initializer : ConstructorDecl.inits()) {
> +    Matches = match(AllDeclRefs, *Initializer->getInit(), Context);
> ----------------
> Should be *Initializer instead of * Initializer.
>
> ================
> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120
> @@ +119,3 @@
> +  }
> +  diag(InitArg->getLocStart(), "value parameter can be moved to avoid
> copy.");
> +}
> ----------------
> Perhaps: "argument can be moved to avoid a copy" instead?
>
> ================
> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:84
> @@ +83,3 @@
> +  Movable(const Movable&) = default;
> +  Movable& operator =(const Movable&) = default;
> +  ~Movable() {}
> ----------------
> Formatting
>
> ================
> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:85
> @@ +84,3 @@
> +  Movable& operator =(const Movable&) = default;
> +  ~Movable() {}
> +};
> ----------------
> Why not = default?
>
> ================
> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:113
> @@ +112,3 @@
> +
> +struct NegativeParamTriviallyCopyable {
> +  NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}
> ----------------
> Should also have a test for scalars
>
>
> http://reviews.llvm.org/D12839
>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150916/18b9fb28/attachment-0001.html>