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

[Aaron Ballman via cfe-commits]

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?

~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