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

[Aaron Ballman via cfe-commits]

Okay, I'm sold on this being a clang-tidy check instead, thank you
both for weighing in!

I would love to see a check that covers more than just constructor
initialization contexts, which suggests a separate checker from
misc-move-constructor-init so that we don't have the same general idea
split over two checkers. For instance, I would love to see this sort
of code flagged along with the cases you've identified:

struct SomethingBig { /* Stuff */ };
struct S {
  std::vector<SomethingBig> V;
};

void f(S &s) {
  std::vector<SomethingBig> V;
  // Fill out a bunch of elements in V
  s.V = V; // suggest std::move()
}

Btw, I'm not suggesting you have to implement this sort of
functionality as part of your patch (but it would be awesome if you
were able to!). I'm only thinking about separation of concerns for
this.

~Aaron

On Wed, Sep 16, 2015 at 3:44 AM, Alexander Kornienko <alexfh at google.com> wrote:
> I also think this suits better for a clang-tidy check, but for a different
> reason: adding `std::move` calls can require adding `#include <utility>`,
> which is fine for a clang-tidy check (and we have facilities for that), but
> it's a questionable functionality for a compiler diagnostic.
>
> On 16 Sep 2015 09:20, "Richard Smith" <richard at metafoo.co.uk> wrote:
>>
>> 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
>>>
>>>
>>>
>>
>