diff --git a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp --- a/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/MoveConstArgCheck.cpp @@ -103,6 +103,7 @@ AlreadyCheckedMoves.insert(CallMove); const Expr *Arg = CallMove->getArg(0); + const QualType ArgType = Arg->getType().getCanonicalType(); SourceManager &SM = Result.Context->getSourceManager(); CharSourceRange MoveRange = @@ -112,12 +113,11 @@ if (!FileMoveRange.isValid()) return; - bool IsConstArg = Arg->getType().isConstQualified(); - bool IsTriviallyCopyable = - Arg->getType().isTriviallyCopyableType(*Result.Context); + bool IsConstArg = ArgType.isConstQualified(); + bool IsTriviallyCopyable = ArgType.isTriviallyCopyableType(*Result.Context); if (IsConstArg || IsTriviallyCopyable) { - if (const CXXRecordDecl *R = Arg->getType()->getAsCXXRecordDecl()) { + if (const CXXRecordDecl *R = ArgType->getAsCXXRecordDecl()) { // According to [expr.prim.lambda]p3, "whether the closure type is // trivially copyable" property can be changed by the implementation of // the language, so we shouldn't rely on it when issuing diagnostics. @@ -196,11 +196,28 @@ if ((*InvocationParmType)->isRValueReferenceType()) return; - auto Diag = diag(FileMoveRange.getBegin(), - "passing result of std::move() as a const reference " - "argument; no move will actually happen"); + { + auto Diag = diag(FileMoveRange.getBegin(), + "passing result of std::move() as a const reference " + "argument; no move will actually happen"); + + replaceCallWithArg(CallMove, Diag, SM, getLangOpts()); + } - replaceCallWithArg(CallMove, Diag, SM, getLangOpts()); + if (const CXXRecordDecl *RecordDecl = ArgType->getAsCXXRecordDecl(); + RecordDecl && !(RecordDecl->hasMoveConstructor() && + RecordDecl->hasMoveAssignment())) { + const bool MissingMoveAssignment = !RecordDecl->hasMoveAssignment(); + const bool MissingMoveConstructor = !RecordDecl->hasMoveConstructor(); + const bool MissingBoth = MissingMoveAssignment && MissingMoveConstructor; + + diag(RecordDecl->getLocation(), + "%0 is not move " + "%select{|assignable}1%select{|/}2%select{|constructible}3", + DiagnosticIDs::Note) + << RecordDecl << MissingMoveAssignment << MissingBoth + << MissingMoveConstructor; + } } } diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -374,6 +374,10 @@ `IgnoreTemplateInstantiations` option to optionally ignore virtual function overrides that are part of template instantiations. +- Improved :doc:`performance-move-const-arg + ` check to warn when move + special member functions are not available. + - Improved :doc:`performance-no-automatic-move `: warn on ``const &&`` constructors. diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp --- a/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/performance/move-const-arg.cpp @@ -39,10 +39,14 @@ A(A &&rhs) {} }; +using AlsoA = A; + struct TriviallyCopyable { int i; }; +using TrivialAlias = TriviallyCopyable; + void f(TriviallyCopyable) {} void g() { @@ -50,6 +54,11 @@ f(std::move(obj)); // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable' has no effect; remove std::move() [performance-move-const-arg] // CHECK-FIXES: f(obj); + + TrivialAlias obj2; + f(std::move(obj2)); + // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: std::move of the variable 'obj2' of the trivially-copyable type 'TrivialAlias' (aka 'TriviallyCopyable') has no effect; remove std::move() [performance-move-const-arg] + // CHECK-FIXES: f(obj2); } int f1() { @@ -72,12 +81,20 @@ A f4(A x4) { return std::move(x4); } +AlsoA f4_a(AlsoA x4) { return std::move(x4); } + A f5(const A x5) { return std::move(x5); // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable 'x5' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg] // CHECK-FIXES: return x5; } +AlsoA f5_a(const AlsoA x5) { + return std::move(x5); + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the const variable 'x5' has no effect; remove std::move() or make the variable non-const [performance-move-const-arg] + // CHECK-FIXES: return x5; +} + template T f6(const T x6) { return std::move(x6); @@ -115,6 +132,8 @@ NoMoveSemantics &operator=(const NoMoveSemantics &); }; +using NoMoveSemanticsAlias = NoMoveSemantics; + void callByConstRef(const NoMoveSemantics &); void callByConstRef(int i, const NoMoveSemantics &); @@ -147,6 +166,35 @@ // CHECK-FIXES: other = obj; } +void moveToConstReferencePositivesAlias() { + NoMoveSemanticsAlias obj; + + // Basic case. + callByConstRef(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-FIXES: callByConstRef(obj); + + // Also works for second argument. + callByConstRef(1, std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:21: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-FIXES: callByConstRef(1, obj); + + // Works if std::move() applied to a temporary. + callByConstRef(std::move(NoMoveSemanticsAlias())); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-FIXES: callByConstRef(NoMoveSemanticsAlias()); + + // Works if calling a copy constructor. + NoMoveSemanticsAlias other(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-FIXES: NoMoveSemanticsAlias other(obj); + + // Works if calling assignment operator. + other = std::move(obj); + // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-FIXES: other = obj; +} + class MoveSemantics { public: MoveSemantics(); @@ -155,6 +203,8 @@ MoveSemantics &operator=(MoveSemantics &&); }; +using MoveSemanticsAlias = MoveSemantics; + void callByValue(MoveSemantics); void callByRValueRef(MoveSemantics &&); @@ -197,6 +247,32 @@ auto lambda2 = std::move(lambda); } +void moveToConstReferenceNegativesAlias() { + // No warning when actual move takes place. + MoveSemanticsAlias move_semantics; + callByValue(std::move(move_semantics)); + callByRValueRef(std::move(move_semantics)); + MoveSemanticsAlias other(std::move(move_semantics)); + other = std::move(move_semantics); + + // No warning if std::move() not used. + NoMoveSemanticsAlias no_move_semantics; + callByConstRef(no_move_semantics); + + // No warning if instantiating a template. + templateFunction(no_move_semantics); + + // No warning inside of macro expansions. + M3(NoMoveSemanticsAlias, no_move_semantics); + + // No warning inside of macro expansion, even if the macro expansion is inside + // a lambda that is, in turn, an argument to a macro. + CALL([no_move_semantics] { M3(NoMoveSemanticsAlias, no_move_semantics); }); + + auto lambda = [] {}; + auto lambda2 = std::move(lambda); +} + class MoveOnly { public: MoveOnly(const MoveOnly &other) = delete; @@ -210,6 +286,8 @@ Q(std::move(val)); } +using MoveOnlyAlias = MoveOnly; + void fmovable(MoveSemantics); void lambda1() { @@ -236,6 +314,12 @@ callback(std::move(m)); } +void functionInvocationAlias() { + function callback; + MoveSemanticsAlias m; + callback(std::move(m)); +} + void lambda2() { function callback; @@ -247,6 +331,17 @@ f(MoveSemantics()); } +void lambda2Alias() { + function callback; + + auto f = [callback = std::move(callback)](MoveSemanticsAlias m) mutable { + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: std::move of the variable 'callback' of the trivially-copyable type 'function' (aka 'function') has no effect; remove std::move() [performance-move-const-arg] + // CHECK-FIXES: auto f = [callback = callback](MoveSemanticsAlias m) mutable { + callback(std::move(m)); + }; + f(MoveSemanticsAlias()); +} + void showInt(int &&v); void showInt(int v1, int &&v2); void showPointer(const char *&&s); @@ -266,7 +361,7 @@ showPointer(std::move(s)); // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg] // CHECK-MESSAGES: :[[@LINE-16]]:32: note: consider changing the 1st parameter of 'showPointer' from 'const char *&&' to 'const char *' - showPointer2(std::move(s)); + showPointer2(std::move(s)); // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg] // CHECK-MESSAGES: :[[@LINE-18]]:39: note: consider changing the 1st parameter of 'showPointer2' from 'const char *const &&' to 'const char *const' TriviallyCopyable *obj = new TriviallyCopyable(); @@ -276,6 +371,13 @@ showTriviallyCopyablePointer(std::move(obj)); // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg] // CHECK-MESSAGES: :[[@LINE-23]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *' + TrivialAlias* obj2 = new TrivialAlias(); + showTriviallyCopyable(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-28]]:48: note: consider changing the 1st parameter of 'showTriviallyCopyable' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + showTriviallyCopyablePointer(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-30]]:62: note: consider changing the 1st parameter of 'showTriviallyCopyablePointer' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *' } template void forwardToShowInt(T && t) { @@ -297,32 +399,62 @@ void showTmp(TriviallyCopyable&& t); static void showTmpStatic(TriviallyCopyable&& t); }; +using TmpAlias = Tmp; + void testMethods() { Tmp t; int a = 10; Tmp t1(std::move(a)); // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] - // CHECK-MESSAGES: :[[@LINE-13]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &' + // CHECK-MESSAGES: :[[@LINE-15]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &' Tmp t2(a, std::move(a)); // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] - // CHECK-MESSAGES: :[[@LINE-15]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &' + // CHECK-MESSAGES: :[[@LINE-17]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &' const char* s = ""; Tmp t3(std::move(s)); // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg] - // CHECK-MESSAGES: :[[@LINE-18]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *' + // CHECK-MESSAGES: :[[@LINE-20]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *' TriviallyCopyable *obj = new TriviallyCopyable(); Tmp t4(std::move(*obj)); // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] - // CHECK-MESSAGES: :[[@LINE-21]]:27: note: consider changing the 1st parameter of 'Tmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + // CHECK-MESSAGES: :[[@LINE-23]]:27: note: consider changing the 1st parameter of 'Tmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' Tmp t5(std::move(obj)); // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: std::move of the variable 'obj' of the trivially-copyable type 'TriviallyCopyable *' has no effect [performance-move-const-arg] - // CHECK-MESSAGES: :[[@LINE-23]]:34: note: consider changing the 1st parameter of 'Tmp' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *' + // CHECK-MESSAGES: :[[@LINE-25]]:34: note: consider changing the 1st parameter of 'Tmp' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *' t.showTmp(std::move(*obj)); // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] - // CHECK-MESSAGES: :[[@LINE-25]]:36: note: consider changing the 1st parameter of 'showTmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + // CHECK-MESSAGES: :[[@LINE-27]]:36: note: consider changing the 1st parameter of 'showTmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' Tmp::showTmpStatic(std::move(*obj)); // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: std::move of the expression of the trivially-copyable type 'TriviallyCopyable' has no effect [performance-move-const-arg] - // CHECK-MESSAGES: :[[@LINE-27]]:49: note: consider changing the 1st parameter of 'showTmpStatic' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + // CHECK-MESSAGES: :[[@LINE-29]]:49: note: consider changing the 1st parameter of 'showTmpStatic' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' +} + +void testMethodsAlias() { + TmpAlias t; + int a = 10; + TmpAlias t1(std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-43]]:13: note: consider changing the 1st parameter of 'Tmp' from 'int &&' to 'const int &' + TmpAlias t2(a, std::move(a)); + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-45]]:21: note: consider changing the 2nd parameter of 'Tmp' from 'int &&' to 'const int &' + const char* s = ""; + TmpAlias t3(std::move(s)); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 's' of the trivially-copyable type 'const char *' has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-48]]:21: note: consider changing the 1st parameter of 'Tmp' from 'const char *&&' to 'const char *' + TrivialAlias *obj = new TrivialAlias(); + TmpAlias t4(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the expression of the trivially-copyable type 'TrivialAlias' (aka 'TriviallyCopyable') has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-51]]:27: note: consider changing the 1st parameter of 'Tmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + TmpAlias t5(std::move(obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:15: warning: std::move of the variable 'obj' of the trivially-copyable type 'TrivialAlias *' (aka 'TriviallyCopyable *') has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-53]]:34: note: consider changing the 1st parameter of 'Tmp' from 'const TriviallyCopyable *&&' to 'const TriviallyCopyable *' + t.showTmp(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:13: warning: std::move of the expression of the trivially-copyable type 'TrivialAlias' (aka 'TriviallyCopyable') has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-55]]:36: note: consider changing the 1st parameter of 'showTmp' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' + TmpAlias::showTmpStatic(std::move(*obj)); + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: std::move of the expression of the trivially-copyable type 'TrivialAlias' (aka 'TriviallyCopyable') has no effect [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-57]]:49: note: consider changing the 1st parameter of 'showTmpStatic' from 'TriviallyCopyable &&' to 'const TriviallyCopyable &' } void showA(A &&v) {} @@ -331,6 +463,11 @@ showA(std::move(a)); } +void testAAlias() { + AlsoA a; + showA(std::move(a)); +} + void testFuncPointer() { int a = 10; void (*choice)(int, int &&); @@ -340,3 +477,68 @@ // CHECK-FIXES: choice(a, std::move(a)); // CHECK-MESSAGES: :[[@LINE-3]]:24: warning: std::move of the variable 'a' of the trivially-copyable type 'int' has no effect [performance-move-const-arg] } + +namespace issue_62550 { + +struct NonMoveConstructable { + NonMoveConstructable(); + NonMoveConstructable(const NonMoveConstructable&); + NonMoveConstructable& operator=(const NonMoveConstructable&); + NonMoveConstructable& operator=(NonMoveConstructable&&); +}; + +void testNonMoveConstructible() { + NonMoveConstructable t1; + NonMoveConstructable t2{std::move(t1)}; + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-11]]:8: note: 'NonMoveConstructable' is not move constructible +} + +struct NonMoveAssignable { + NonMoveAssignable(); + NonMoveAssignable(const NonMoveAssignable&); + NonMoveAssignable(NonMoveAssignable&&); + + NonMoveAssignable& operator=(const NonMoveAssignable&); +}; + +void testNonMoveAssignable() { + NonMoveAssignable t1; + NonMoveAssignable t2; + + t2 = std::move(t1); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-14]]:8: note: 'NonMoveAssignable' is not move assignable +} + +struct NonMoveable { + NonMoveable(); + NonMoveable(const NonMoveable&); + NonMoveable& operator=(const NonMoveable&); +}; + +void testNonMoveable() { + NonMoveable t1; + NonMoveable t2{std::move(t1)}; + // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-10]]:8: note: 'NonMoveable' is not move assignable/constructible + + t1 = std::move(t2); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-14]]:8: note: 'NonMoveable' is not move assignable/constructible +} + +using AlsoNonMoveable = NonMoveable; + +void testAlsoNonMoveable() { + AlsoNonMoveable t1; + AlsoNonMoveable t2{std::move(t1)}; + // CHECK-MESSAGES: :[[@LINE-1]]:22: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-23]]:8: note: 'NonMoveable' is not move assignable/constructible + + t1 = std::move(t2); + // CHECK-MESSAGES: :[[@LINE-1]]:8: warning: passing result of std::move() as a const reference argument; no move will actually happen [performance-move-const-arg] + // CHECK-MESSAGES: :[[@LINE-27]]:8: note: 'NonMoveable' is not move assignable/constructible +} + +} // namespace issue_62550