<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Wed, Sep 16, 2015 at 5:15 PM, Aaron Ballman <span dir="ltr"><<a href="mailto:aaron.ballman@gmail.com" target="_blank">aaron.ballman@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Okay, I'm sold on this being a clang-tidy check instead, thank you<br>
both for weighing in!<br>
<br>
I would love to see a check that covers more than just constructor<br>
initialization contexts, which suggests a separate checker from<br>
misc-move-constructor-init so that we don't have the same general idea<br>
split over two checkers. For instance, I would love to see this sort<br>
of code flagged along with the cases you've identified:<br>
<br>
struct SomethingBig { /* Stuff */ };<br>
struct S {<br>
  std::vector<SomethingBig> V;<br>
};<br>
<br>
void f(S &s) {<br>
  std::vector<SomethingBig> V;<br>
  // Fill out a bunch of elements in V<br>
  s.V = V; // suggest std::move()<br>
}<br></blockquote><div><br></div><div>Detecting that the local variable is not used after something is initialized with it will require analysis of the control flow graph in the general case, which I'm not sure is currently possible in a clang-tidy check (specifically, I'm not sure whether we can make Sema available to checks).</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Btw, I'm not suggesting you have to implement this sort of<br>
functionality as part of your patch (but it would be awesome if you<br>
were able to!). I'm only thinking about separation of concerns for<br>
this.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Wed, Sep 16, 2015 at 3:44 AM, Alexander Kornienko <<a href="mailto:alexfh@google.com">alexfh@google.com</a>> wrote:<br>
> I also think this suits better for a clang-tidy check, but for a different<br>
> reason: adding `std::move` calls can require adding `#include <utility>`,<br>
> which is fine for a clang-tidy check (and we have facilities for that), but<br>
> it's a questionable functionality for a compiler diagnostic.<br>
><br>
> On 16 Sep 2015 09:20, "Richard Smith" <<a href="mailto:richard@metafoo.co.uk">richard@metafoo.co.uk</a>> wrote:<br>
>><br>
>> On Tue, Sep 15, 2015 at 12:50 PM, Aaron Ballman <<a href="mailto:aaron.ballman@gmail.com">aaron.ballman@gmail.com</a>><br>
>> wrote:<br>
>>><br>
>>> aaron.ballman added reviewers: rtrieu, rsmith.<br>
>>> aaron.ballman added a comment.<br>
>>><br>
>>> There is a -Wpessimizing-move frontend warning that Richard added not too<br>
>>> long ago, which tells the user to remove calls to std::move() because they<br>
>>> pessimize the code. The new functionality you are looking to add is<br>
>>> basically the opposite: it tells the user to add std::move() because the<br>
>>> code is currently pessimized due to copies. Given how general that concept<br>
>>> is (it doesn't just appertain to constructor initializations), I wonder if<br>
>>> this makes more sense as a frontend warning like -Wpessimizing-copy.<br>
>>><br>
>>> Richard (both of you), what do you think?<br>
>><br>
>><br>
>> I think this is in the grey area between what's appropriate for clang-tidy<br>
>> and what's appropriate as a warning, where both options have merit; the same<br>
>> is true with -Wpessimizing-move. I think the difference between the two<br>
>> cases is that -Wpessimizing-move detects a case where you wrote something<br>
>> that doesn't do what (we think) you meant, whereas this check detects a case<br>
>> where you /didn't/ write something that (we think) would make your code<br>
>> better (even though both changes have similar effects, of safely turning a<br>
>> copy into a move or a move into an elided move). It's a fine line, but for<br>
>> me that nudges -Wpessimizing-move into a warning, and this check into<br>
>> clang-tidy territory.<br>
>><br>
>>> ~Aaron<br>
>>><br>
>>><br>
>>> ================<br>
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:32<br>
>>> @@ +31,3 @@<br>
>>> +  // excessive copying, we'll warn on them.<br>
>>> +  if (Node->isDependentType()) {<br>
>>> +    return false;<br>
>>> ----------------<br>
>>> Elide braces, here and elsewhere.<br>
>>><br>
>>> ================<br>
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:36<br>
>>> @@ +35,3 @@<br>
>>> +  // Ignore trivially copyable types.<br>
>>> +  if (Node->isScalarType() ||<br>
>>> +      Node.isTriviallyCopyableType(Finder->getASTContext()) ||<br>
>>> ----------------<br>
>>> Can turn this into a return statement directly instead of an if.<br>
>>><br>
>>> ================<br>
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:38<br>
>>> @@ +37,3 @@<br>
>>> +      Node.isTriviallyCopyableType(Finder->getASTContext()) ||<br>
>>> +      classHasTrivialCopyAndDestroy(Node)) {<br>
>>> +    return false;<br>
>>> ----------------<br>
>>> Why do you need classHasTrivialCopyAndDestroy() when you're already<br>
>>> checking if it's a trivially copyable type?<br>
>>><br>
>>> ================<br>
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:44<br>
>>> @@ +43,3 @@<br>
>>> +<br>
>>> +int parmVarDeclRefExprOccurences(const ParmVarDecl &MovableParam,<br>
>>> +                                 const CXXConstructorDecl<br>
>>> &ConstructorDecl,<br>
>>> ----------------<br>
>>> Should return unsigned, please.<br>
>>><br>
>>> ================<br>
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:50<br>
>>> @@ +49,3 @@<br>
>>> +      findAll(declRefExpr(to(parmVarDecl(equalsNode(&MovableParam)))));<br>
>>> +  auto Matches = match(AllDeclRefs, *ConstructorDecl.getBody(),<br>
>>> Context);<br>
>>> +  Occurrences += Matches.size();<br>
>>> ----------------<br>
>>> You don't actually need Matches, you can call match().size() instead.<br>
>>><br>
>>> ================<br>
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:52<br>
>>> @@ +51,3 @@<br>
>>> +  Occurrences += Matches.size();<br>
>>> +  for (const auto* Initializer : ConstructorDecl.inits()) {<br>
>>> +    Matches = match(AllDeclRefs, *Initializer->getInit(), Context);<br>
>>> ----------------<br>
>>> Should be *Initializer instead of * Initializer.<br>
>>><br>
>>> ================<br>
>>> Comment at: clang-tidy/misc/MoveConstructorInitCheck.cpp:120<br>
>>> @@ +119,3 @@<br>
>>> +  }<br>
>>> +  diag(InitArg->getLocStart(), "value parameter can be moved to avoid<br>
>>> copy.");<br>
>>> +}<br>
>>> ----------------<br>
>>> Perhaps: "argument can be moved to avoid a copy" instead?<br>
>>><br>
>>> ================<br>
>>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:84<br>
>>> @@ +83,3 @@<br>
>>> +  Movable(const Movable&) = default;<br>
>>> +  Movable& operator =(const Movable&) = default;<br>
>>> +  ~Movable() {}<br>
>>> ----------------<br>
>>> Formatting<br>
>>><br>
>>> ================<br>
>>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:85<br>
>>> @@ +84,3 @@<br>
>>> +  Movable& operator =(const Movable&) = default;<br>
>>> +  ~Movable() {}<br>
>>> +};<br>
>>> ----------------<br>
>>> Why not = default?<br>
>>><br>
>>> ================<br>
>>> Comment at: test/clang-tidy/misc-move-constructor-init.cpp:113<br>
>>> @@ +112,3 @@<br>
>>> +<br>
>>> +struct NegativeParamTriviallyCopyable {<br>
>>> +  NegativeParamTriviallyCopyable(TriviallyCopyable T) : T_(T) {}<br>
>>> ----------------<br>
>>> Should also have a test for scalars<br>
>>><br>
>>><br>
>>> <a href="http://reviews.llvm.org/D12839" rel="noreferrer" target="_blank">http://reviews.llvm.org/D12839</a><br>
>>><br>
>>><br>
>>><br>
>><br>
><br>
</div></div></blockquote></div><div><br></div>
</div></div>