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

[Alexander Kornienko via cfe-commits]

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
>>
>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150916/5d0da38f/attachment.html>