<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Sep 15, 2015 at 12:50 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:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">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 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.<br>
<br>
Richard (both of you), what do you think?<br></blockquote><div><br></div><div>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.</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
~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 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 &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(), 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 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>
</blockquote></div><br></div></div>