[clang-tidy] modernize-pass-by-value bugfix

Modified the clang-tidy PassByValue check. It now stops adding std::move to type which is trivially copyable because that caused the clang-tidy MoveConstArg to complain and revert, thus creating a cycle.

I have also added a lit-style test to verify the bugfix.

This is the bug on bugzilla: https://llvm.org/bugs/show_bug.cgi?id=27731

This is the code review on phabricator: http://reviews.llvm.org/D20365

llvm-svn: 270565

2 files changed, 30 insertions, 15 deletions

diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
index c727deb7969..b8f83bbd7c1 100644
--- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -181,6 +181,11 @@ void PassByValueCheck::check(const MatchFinder::MatchResult &Result) {
   if (!paramReferredExactlyOnce(Ctor, ParamDecl))
     return;
 
+  // If the parameter is trivial to copy, don't move it. Moving a trivivally
+  // copyable type will cause a problem with misc-move-const-arg
+  if (ParamDecl->getType().isTriviallyCopyableType(*Result.Context)) 
+    return;
+
   auto Diag = diag(ParamDecl->getLocStart(), "pass by value and use std::move");
 
   // Iterate over all declarations of the constructor.
diff --git a/clang-tools-extra/test/clang-tidy/modernize-pass-by-value.cpp b/clang-tools-extra/test/clang-tidy/modernize-pass-by-value.cpp
index 724b1f69cfe..b45fd83ee55 100644
--- a/clang-tools-extra/test/clang-tidy/modernize-pass-by-value.cpp
+++ b/clang-tools-extra/test/clang-tidy/modernize-pass-by-value.cpp
@@ -1,3 +1,6 @@
+#include <utility>
+#include <array>
+
 // RUN: %check_clang_tidy %s modernize-pass-by-value %t -- -- -std=c++11 -fno-delayed-template-parsing
 
 // CHECK-FIXES: #include <utility>
@@ -17,7 +20,7 @@ struct NotMovable {
 }
 
 struct A {
-  A(const Movable &M) : M(M) {}
+  A(Movable M) : M(std::move(M)) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move [modernize-pass-by-value]
   // CHECK-FIXES: A(Movable M) : M(std::move(M)) {}
   Movable M;
@@ -46,17 +49,17 @@ struct C {
 
 // Test that both declaration and definition are updated.
 struct D {
-  D(const Movable &M);
+  D(Movable M);
   // CHECK-FIXES: D(Movable M);
   Movable M;
 };
-D::D(const Movable &M) : M(M) {}
+D::D(Movable M) : M(std::move(M)) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
 // CHECK-FIXES: D::D(Movable M) : M(std::move(M)) {}
 
 // Test with default parameter.
 struct E {
-  E(const Movable &M = Movable()) : M(M) {}
+  E(Movable M = Movable()) : M(std::move(M)) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
   // CHECK-FIXES: E(Movable M = Movable()) : M(std::move(M)) {}
   Movable M;
@@ -71,11 +74,11 @@ struct F {
 
 // Test unnamed parameter in declaration.
 struct G {
-  G(const Movable &);
+  G(Movable );
   // CHECK-FIXES: G(Movable );
   Movable M;
 };
-G::G(const Movable &M) : M(M) {}
+G::G(Movable M) : M(std::move(M)) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
 // CHECK-FIXES: G::G(Movable M) : M(std::move(M)) {}
 
@@ -84,12 +87,12 @@ namespace ns_H {
 typedef ::Movable HMovable;
 }
 struct H {
-  H(const ns_H::HMovable &M);
+  H(ns_H::HMovable M);
   // CHECK-FIXES: H(ns_H::HMovable M);
   ns_H::HMovable M;
 };
 using namespace ns_H;
-H::H(const HMovable &M) : M(M) {}
+H::H(HMovable M) : M(std::move(M)) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
 // CHECK-FIXES: H(HMovable M) : M(std::move(M)) {}
 
@@ -122,14 +125,14 @@ struct K_Movable {
 
 // Test with movable type with an user defined move constructor.
 struct K {
-  K(const K_Movable &M) : M(M) {}
+  K(K_Movable M) : M(std::move(M)) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
   // CHECK-FIXES: K(K_Movable M) : M(std::move(M)) {}
   K_Movable M;
 };
 
 template <typename T> struct L {
-  L(const Movable &M) : M(M) {}
+  L(Movable M) : M(std::move(M)) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
   // CHECK-FIXES: L(Movable M) : M(std::move(M)) {}
   Movable M;
@@ -138,7 +141,7 @@ L<int> l(Movable());
 
 // Test with a non-instantiated template class.
 template <typename T> struct N {
-  N(const Movable &M) : M(M) {}
+  N(Movable M) : M(std::move(M)) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
   // CHECK-FIXES: N(Movable M) : M(std::move(M)) {}
 
@@ -148,7 +151,7 @@ template <typename T> struct N {
 
 // Test with value parameter.
 struct O {
-  O(Movable M) : M(M) {}
+  O(Movable M) : M(std::move(M)) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
   // CHECK-FIXES: O(Movable M) : M(std::move(M)) {}
   Movable M;
@@ -164,8 +167,8 @@ struct P {
 // Test with multiples parameters where some need to be changed and some don't.
 // need to.
 struct Q {
-  Q(const Movable &A, const Movable &B, const Movable &C, double D)
-      : A(A), B(B), C(C), D(D) {}
+  Q(const Movable &A, Movable B, Movable C, double D)
+      : A(A), B(std::move(B)), C(std::move(C)), D(D) {}
   // CHECK-MESSAGES: :[[@LINE-2]]:23: warning: pass by value and use std::move
   // CHECK-MESSAGES: :[[@LINE-3]]:41: warning: pass by value and use std::move
   // CHECK-FIXES:      Q(const Movable &A, Movable B, Movable C, double D)
@@ -181,7 +184,7 @@ namespace ns_R {
 typedef ::Movable RMovable;
 }
 struct R {
-  R(ns_R::RMovable M) : M(M) {}
+  R(ns_R::RMovable M) : M(std::move(M)) {}
   // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: pass by value and use std::move
   // CHECK-FIXES: R(ns_R::RMovable M) : M(std::move(M)) {}
   ns_R::RMovable M;
@@ -193,3 +196,10 @@ struct S {
   // CHECK-FIXES: S(Movable &&M) : M(M) {}
   Movable M;
 };
+
+// Test that types that are trivially copyable will not use std::move. This will
+// cause problems with misc-move-const-arg, as it will revert it.
+struct T {
+  std::array<int, 10> a_;
+  T(std::array<int, 10> a) : a_(a) {}
+};