Author: madsravn Date: Tue May 24 10:00:16 2016 New Revision: 270565 URL: http://llvm.org/viewvc/llvm-project?rev=270565&view=rev Log: [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 Modified: clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp clang-tools-extra/trunk/test/clang-tidy/modernize-pass-by-value.cpp Modified: clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp?rev=270565&r1=270564&r2=270565&view=diff ============================================================================== --- clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp (original) +++ clang-tools-extra/trunk/clang-tidy/modernize/PassByValueCheck.cpp Tue May 24 10:00:16 2016 @@ -181,6 +181,11 @@ void PassByValueCheck::check(const Match 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. Modified: clang-tools-extra/trunk/test/clang-tidy/modernize-pass-by-value.cpp URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/test/clang-tidy/modernize-pass-by-value.cpp?rev=270565&r1=270564&r2=270565&view=diff ============================================================================== --- clang-tools-extra/trunk/test/clang-tidy/modernize-pass-by-value.cpp (original) +++ clang-tools-extra/trunk/test/clang-tidy/modernize-pass-by-value.cpp Tue May 24 10:00:16 2016 @@ -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) {} +}; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
