From 321f24ec42f65403134031bbe99ad3c468b01efe Mon Sep 17 00:00:00 2001 From: Malcolm Parsons Date: Thu, 12 Apr 2018 14:48:48 +0000 Subject: [PATCH] Diagnose cases of "return x" that should be "return std::move(x)" for efficiency Summary: This patch adds two new diagnostics, which are off by default: **-Wreturn-std-move** This diagnostic is enabled by `-Wreturn-std-move`, `-Wmove`, or `-Wall`. Diagnose cases of `return x` or `throw x`, where `x` is the name of a local variable or parameter, in which a copy operation is performed when a move operation would have been available. The user probably expected a move, but they're not getting a move, perhaps because the type of "x" is different from the return type of the function. A place where this comes up in the wild is `stdext::inplace_function` which implements conversion via a conversion operator rather than a converting constructor; see https://github.com/WG21-SG14/SG14/issues/125#issue-297201412 Another place where this has come up in the wild, but where the fix ended up being different, was try { ... } catch (ExceptionType ex) { throw ex; } where the appropriate fix in that case was to replace `throw ex;` with `throw;`, and incidentally to catch by reference instead of by value. (But one could contrive a scenario where the slicing was intentional, in which case throw-by-move would have been the appropriate fix after all.) Another example (intentional slicing to a base class) is dissected in https://github.com/accuBayArea/Slides/blob/master/slides/2018-03-07.pdf **-Wreturn-std-move-in-c++11** This diagnostic is enabled only by the exact spelling `-Wreturn-std-move-in-c++11`. Diagnose cases of "return x;" or "throw x;" which in this version of Clang *do* produce moves, but which prior to Clang 3.9 / GCC 5.1 produced copies instead. This is useful in codebases which care about portability to those older compilers. The name "-in-c++11" is not technically correct; what caused the version-to-version change in behavior here was actually CWG 1579, not C++14. I think it's likely that codebases that need portability to GCC 4.9-and-earlier may understand "C++11" as a colloquialism for "older compilers." The wording of this diagnostic is based on feedback from @rsmith. **Discussion** Notice that this patch is kind of a negative-space version of Richard Trieu's `-Wpessimizing-move`. That diagnostic warns about cases of `return std::move(x)` that should be `return x` for speed. These diagnostics warn about cases of `return x` that should be `return std::move(x)` for speed. (The two diagnostics' bailiwicks do not overlap: we don't have to worry about a `return` statement flipping between the two states indefinitely.) I propose to write a paper for San Diego that would relax the implicit-move rules so that in C++2a the user //would// see the moves they expect, and the diagnostic could be re-worded in a later version of Clang to suggest explicit `std::move` only "in C++17 and earlier." But in the meantime (and/or forever if that proposal is not well received), this diagnostic will be useful to detect accidental copy operations. Reviewers: rtrieu, rsmith Reviewed By: rsmith Subscribers: lebedev.ri, Rakete1111, rsmith, cfe-commits Tags: #clang Differential Revision: https://reviews.llvm.org/D43322 Patch by Arthur O'Dwyer. llvm-svn: 329914 --- clang/include/clang/Basic/DiagnosticGroups.td | 11 +- clang/include/clang/Basic/DiagnosticSemaKinds.td | 13 + clang/include/clang/Sema/Sema.h | 4 + clang/lib/Sema/SemaStmt.cpp | 127 +++++++-- clang/test/SemaCXX/warn-return-std-move.cpp | 334 +++++++++++++++++++++++ 5 files changed, 471 insertions(+), 18 deletions(-) create mode 100644 clang/test/SemaCXX/warn-return-std-move.cpp diff --git a/clang/include/clang/Basic/DiagnosticGroups.td b/clang/include/clang/Basic/DiagnosticGroups.td index 0a3e563..a70197c 100644 --- a/clang/include/clang/Basic/DiagnosticGroups.td +++ b/clang/include/clang/Basic/DiagnosticGroups.td @@ -383,7 +383,11 @@ def DeprecatedObjCIsaUsage : DiagGroup<"deprecated-objc-isa-usage">; def ExplicitInitializeCall : DiagGroup<"explicit-initialize-call">; def Packed : DiagGroup<"packed">; def Padded : DiagGroup<"padded">; + def PessimizingMove : DiagGroup<"pessimizing-move">; +def ReturnStdMoveInCXX11 : DiagGroup<"return-std-move-in-c++11">; +def ReturnStdMove : DiagGroup<"return-std-move">; + def PointerArith : DiagGroup<"pointer-arith">; def PoundWarning : DiagGroup<"#warnings">; def PoundPragmaMessage : DiagGroup<"#pragma-messages">, @@ -723,7 +727,12 @@ def IntToVoidPointerCast : DiagGroup<"int-to-void-pointer-cast">; def IntToPointerCast : DiagGroup<"int-to-pointer-cast", [IntToVoidPointerCast]>; -def Move : DiagGroup<"move", [PessimizingMove, RedundantMove, SelfMove]>; +def Move : DiagGroup<"move", [ + PessimizingMove, + RedundantMove, + ReturnStdMove, + SelfMove + ]>; def Extra : DiagGroup<"extra", [ MissingFieldInitializers, diff --git a/clang/include/clang/Basic/DiagnosticSemaKinds.td b/clang/include/clang/Basic/DiagnosticSemaKinds.td index 9c3ad98..d68afa1 100644 --- a/clang/include/clang/Basic/DiagnosticSemaKinds.td +++ b/clang/include/clang/Basic/DiagnosticSemaKinds.td @@ -5618,6 +5618,19 @@ def warn_pessimizing_move_on_initialization : Warning< InGroup, DefaultIgnore; def note_remove_move : Note<"remove std::move call here">; +def warn_return_std_move : Warning< + "local variable %0 will be copied despite being %select{returned|thrown}1 by name">, + InGroup, DefaultIgnore; +def note_add_std_move : Note< + "call 'std::move' explicitly to avoid copying">; +def warn_return_std_move_in_cxx11 : Warning< + "prior to the resolution of a defect report against ISO C++11, " + "local variable %0 would have been copied despite being returned by name, " + "due to its not matching the function return type%diff{ ($ vs $)|}1,2">, + InGroup, DefaultIgnore; +def note_add_std_move_in_cxx11 : Note< + "call 'std::move' explicitly to avoid copying on older compilers">; + def warn_string_plus_int : Warning< "adding %0 to a string does not append to the string">, InGroup; diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h index 0305094..19f94f0 100644 --- a/clang/include/clang/Sema/Sema.h +++ b/clang/include/clang/Sema/Sema.h @@ -3800,7 +3800,11 @@ public: CES_Strict = 0, CES_AllowParameters = 1, CES_AllowDifferentTypes = 2, + CES_AllowExceptionVariables = 4, + CES_FormerDefault = (CES_AllowParameters), CES_Default = (CES_AllowParameters | CES_AllowDifferentTypes), + CES_AsIfByStdMove = (CES_AllowParameters | CES_AllowDifferentTypes | + CES_AllowExceptionVariables), }; VarDecl *getCopyElisionCandidate(QualType ReturnType, Expr *E, diff --git a/clang/lib/Sema/SemaStmt.cpp b/clang/lib/Sema/SemaStmt.cpp index 8963f4c..c3a627f 100644 --- a/clang/lib/Sema/SemaStmt.cpp +++ b/clang/lib/Sema/SemaStmt.cpp @@ -2905,7 +2905,8 @@ bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD, if (VD->getKind() != Decl::Var && !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar)) return false; - if (VD->isExceptionVariable()) return false; + if (!(CESK & CES_AllowExceptionVariables) && VD->isExceptionVariable()) + return false; // ...automatic... if (!VD->hasLocalStorage()) return false; @@ -2937,6 +2938,11 @@ bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD, /// returned lvalues as rvalues in certain cases (to prefer move construction), /// then falls back to treating them as lvalues if that failed. /// +/// \param ConvertingConstructorsOnly If true, follow [class.copy]p32 and reject +/// resolutions that find non-constructors, such as derived-to-base conversions +/// or `operator T()&&` member functions. If false, do consider such +/// conversion sequences. +/// /// \param Res We will fill this in if move-initialization was possible. /// If move-initialization is not possible, such that we must fall back to /// treating the operand as an lvalue, we will leave Res in its original @@ -2946,6 +2952,7 @@ static void TryMoveInitialization(Sema& S, const VarDecl *NRVOCandidate, QualType ResultType, Expr *&Value, + bool ConvertingConstructorsOnly, ExprResult &Res) { ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(), CK_NoOp, Value, VK_XValue); @@ -2966,22 +2973,39 @@ static void TryMoveInitialization(Sema& S, continue; FunctionDecl *FD = Step.Function.Function; - if (isa(FD)) { - // C++14 [class.copy]p32: - // [...] If the first overload resolution fails or was not performed, - // or if the type of the first parameter of the selected constructor - // is not an rvalue reference to the object's type (possibly - // cv-qualified), overload resolution is performed again, considering - // the object as an lvalue. - const RValueReferenceType *RRefType = - FD->getParamDecl(0)->getType()->getAs(); - if (!RRefType) - break; - if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(), - NRVOCandidate->getType())) - break; + if (ConvertingConstructorsOnly) { + if (isa(FD)) { + // C++14 [class.copy]p32: + // [...] If the first overload resolution fails or was not performed, + // or if the type of the first parameter of the selected constructor + // is not an rvalue reference to the object's type (possibly + // cv-qualified), overload resolution is performed again, considering + // the object as an lvalue. + const RValueReferenceType *RRefType = + FD->getParamDecl(0)->getType()->getAs(); + if (!RRefType) + break; + if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(), + NRVOCandidate->getType())) + break; + } else { + continue; + } } else { - continue; + if (isa(FD)) { + // Check that overload resolution selected a constructor taking an + // rvalue reference. If it selected an lvalue reference, then we + // didn't need to cast this thing to an rvalue in the first place. + if (!isa(FD->getParamDecl(0)->getType())) + break; + } else if (isa(FD)) { + // Check that overload resolution selected a conversion operator + // taking an rvalue reference. + if (cast(FD)->getRefQualifier() != RQ_RValue) + break; + } else { + continue; + } } // Promote "AsRvalue" to the heap, since we now need this @@ -3019,13 +3043,82 @@ Sema::PerformMoveOrCopyInitialization(const InitializedEntity &Entity, ExprResult Res = ExprError(); if (AllowNRVO) { + bool AffectedByCWG1579 = false; + if (!NRVOCandidate) { NRVOCandidate = getCopyElisionCandidate(ResultType, Value, CES_Default); + if (NRVOCandidate && + !getDiagnostics().isIgnored(diag::warn_return_std_move_in_cxx11, + Value->getExprLoc())) { + const VarDecl *NRVOCandidateInCXX11 = + getCopyElisionCandidate(ResultType, Value, CES_FormerDefault); + AffectedByCWG1579 = (!NRVOCandidateInCXX11); + } } if (NRVOCandidate) { TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value, - Res); + true, Res); + } + + if (!Res.isInvalid() && AffectedByCWG1579) { + QualType QT = NRVOCandidate->getType(); + if (QT.getNonReferenceType() + .getUnqualifiedType() + .isTriviallyCopyableType(Context)) { + // Adding 'std::move' around a trivially copyable variable is probably + // pointless. Don't suggest it. + } else { + // Common cases for this are returning unique_ptr from a + // function of return type unique_ptr, or returning T from a + // function of return type Expected. This is totally fine in a + // post-CWG1579 world, but was not fine before. + assert(!ResultType.isNull()); + SmallString<32> Str; + Str += "std::move("; + Str += NRVOCandidate->getDeclName().getAsString(); + Str += ")"; + Diag(Value->getExprLoc(), diag::warn_return_std_move_in_cxx11) + << Value->getSourceRange() + << NRVOCandidate->getDeclName() << ResultType << QT; + Diag(Value->getExprLoc(), diag::note_add_std_move_in_cxx11) + << FixItHint::CreateReplacement(Value->getSourceRange(), Str); + } + } else if (Res.isInvalid() && + !getDiagnostics().isIgnored(diag::warn_return_std_move, + Value->getExprLoc())) { + const VarDecl *FakeNRVOCandidate = + getCopyElisionCandidate(QualType(), Value, CES_AsIfByStdMove); + if (FakeNRVOCandidate) { + QualType QT = FakeNRVOCandidate->getType(); + if (QT->isLValueReferenceType()) { + // Adding 'std::move' around an lvalue reference variable's name is + // dangerous. Don't suggest it. + } else if (QT.getNonReferenceType() + .getUnqualifiedType() + .isTriviallyCopyableType(Context)) { + // Adding 'std::move' around a trivially copyable variable is probably + // pointless. Don't suggest it. + } else { + ExprResult FakeRes = ExprError(); + Expr *FakeValue = Value; + TryMoveInitialization(*this, Entity, FakeNRVOCandidate, ResultType, + FakeValue, false, FakeRes); + if (!FakeRes.isInvalid()) { + bool IsThrow = + (Entity.getKind() == InitializedEntity::EK_Exception); + SmallString<32> Str; + Str += "std::move("; + Str += FakeNRVOCandidate->getDeclName().getAsString(); + Str += ")"; + Diag(Value->getExprLoc(), diag::warn_return_std_move) + << Value->getSourceRange() + << FakeNRVOCandidate->getDeclName() << IsThrow; + Diag(Value->getExprLoc(), diag::note_add_std_move) + << FixItHint::CreateReplacement(Value->getSourceRange(), Str); + } + } + } } } diff --git a/clang/test/SemaCXX/warn-return-std-move.cpp b/clang/test/SemaCXX/warn-return-std-move.cpp new file mode 100644 index 0000000..48542f4 --- /dev/null +++ b/clang/test/SemaCXX/warn-return-std-move.cpp @@ -0,0 +1,334 @@ +// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -Wreturn-std-move -Wreturn-std-move-in-c++11 -std=c++14 -verify %s +// RUN: %clang_cc1 -fsyntax-only -fcxx-exceptions -Wreturn-std-move -Wreturn-std-move-in-c++11 -std=c++14 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s + +// definitions for std::move +namespace std { +inline namespace foo { +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; +template struct remove_reference { typedef T type; }; + +template typename remove_reference::type &&move(T &&t); +} // namespace foo +} // namespace std + +struct Instrument { + Instrument() {} + Instrument(Instrument&&) { /* MOVE */ } + Instrument(const Instrument&) { /* COPY */ } +}; +struct ConvertFromBase { Instrument i; }; +struct ConvertFromDerived { Instrument i; }; +struct Base { + Instrument i; + operator ConvertFromBase() const& { return ConvertFromBase{i}; } + operator ConvertFromBase() && { return ConvertFromBase{std::move(i)}; } +}; +struct Derived : public Base { + operator ConvertFromDerived() const& { return ConvertFromDerived{i}; } + operator ConvertFromDerived() && { return ConvertFromDerived{std::move(i)}; } +}; +struct ConstructFromBase { + Instrument i; + ConstructFromBase(const Base& b): i(b.i) {} + ConstructFromBase(Base&& b): i(std::move(b.i)) {} +}; +struct ConstructFromDerived { + Instrument i; + ConstructFromDerived(const Derived& d): i(d.i) {} + ConstructFromDerived(Derived&& d): i(std::move(d.i)) {} +}; + +struct TrivialInstrument { + int i = 42; +}; +struct ConvertFromTrivialBase { TrivialInstrument i; }; +struct ConvertFromTrivialDerived { TrivialInstrument i; }; +struct TrivialBase { + TrivialInstrument i; + operator ConvertFromTrivialBase() const& { return ConvertFromTrivialBase{i}; } + operator ConvertFromTrivialBase() && { return ConvertFromTrivialBase{std::move(i)}; } +}; +struct TrivialDerived : public TrivialBase { + operator ConvertFromTrivialDerived() const& { return ConvertFromTrivialDerived{i}; } + operator ConvertFromTrivialDerived() && { return ConvertFromTrivialDerived{std::move(i)}; } +}; +struct ConstructFromTrivialBase { + TrivialInstrument i; + ConstructFromTrivialBase(const TrivialBase& b): i(b.i) {} + ConstructFromTrivialBase(TrivialBase&& b): i(std::move(b.i)) {} +}; +struct ConstructFromTrivialDerived { + TrivialInstrument i; + ConstructFromTrivialDerived(const TrivialDerived& d): i(d.i) {} + ConstructFromTrivialDerived(TrivialDerived&& d): i(std::move(d.i)) {} +}; + +Derived test1() { + Derived d1; + return d1; // ok +} +Base test2() { + Derived d2; + return d2; // e1 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d2)" +} +ConstructFromDerived test3() { + Derived d3; + return d3; // e2-cxx11 + // expected-warning@-1{{would have been copied despite being returned by name}} + // expected-note@-2{{to avoid copying on older compilers}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d3)" +} +ConstructFromBase test4() { + Derived d4; + return d4; // e3 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d4)" +} +ConvertFromDerived test5() { + Derived d5; + return d5; // e4 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d5)" +} +ConvertFromBase test6() { + Derived d6; + return d6; // e5 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:14}:"std::move(d6)" +} + +// These test cases should not produce the warning. +Derived ok1() { Derived d; return d; } +Base ok2() { Derived d; return static_cast(d); } +ConstructFromDerived ok3() { Derived d; return static_cast(d); } +ConstructFromBase ok4() { Derived d; return static_cast(d); } +ConvertFromDerived ok5() { Derived d; return static_cast(d); } +ConvertFromBase ok6() { Derived d; return static_cast(d); } + +// If the target is an lvalue reference, assume it's not safe to move from. +Derived ok_plvalue1(Derived& d) { return d; } +Base ok_plvalue2(Derived& d) { return d; } +ConstructFromDerived ok_plvalue3(const Derived& d) { return d; } +ConstructFromBase ok_plvalue4(Derived& d) { return d; } +ConvertFromDerived ok_plvalue5(Derived& d) { return d; } +ConvertFromBase ok_plvalue6(Derived& d) { return d; } + +Derived ok_lvalue1(Derived *p) { Derived& d = *p; return d; } +Base ok_lvalue2(Derived *p) { Derived& d = *p; return d; } +ConstructFromDerived ok_lvalue3(Derived *p) { const Derived& d = *p; return d; } +ConstructFromBase ok_lvalue4(Derived *p) { Derived& d = *p; return d; } +ConvertFromDerived ok_lvalue5(Derived *p) { Derived& d = *p; return d; } +ConvertFromBase ok_lvalue6(Derived *p) { Derived& d = *p; return d; } + +// If the target is a global, assume it's not safe to move from. +static Derived global_d; +Derived ok_global1() { return global_d; } +Base ok_global2() { return global_d; } +ConstructFromDerived ok_global3() { return global_d; } +ConstructFromBase ok_global4() { return global_d; } +ConvertFromDerived ok_global5() { return global_d; } +ConvertFromBase ok_global6() { return global_d; } + +// If the target's copy constructor is trivial, assume the programmer doesn't care. +TrivialDerived ok_trivial1(TrivialDerived d) { return d; } +TrivialBase ok_trivial2(TrivialDerived d) { return d; } +ConstructFromTrivialDerived ok_trivial3(TrivialDerived d) { return d; } +ConstructFromTrivialBase ok_trivial4(TrivialDerived d) { return d; } +ConvertFromTrivialDerived ok_trivial5(TrivialDerived d) { return d; } +ConvertFromTrivialBase ok_trivial6(TrivialDerived d) { return d; } + +// If the target is a parameter, do apply the diagnostic. +Derived testParam1(Derived d) { return d; } +Base testParam2(Derived d) { + return d; // e6 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConstructFromDerived testParam3(Derived d) { + return d; // e7-cxx11 + // expected-warning@-1{{would have been copied despite being returned by name}} + // expected-note@-2{{to avoid copying on older compilers}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConstructFromBase testParam4(Derived d) { + return d; // e8 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConvertFromDerived testParam5(Derived d) { + return d; // e9 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConvertFromBase testParam6(Derived d) { + return d; // e10 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} + +// If the target is an rvalue reference parameter, do apply the diagnostic. +Derived testRParam1(Derived&& d) { + return d; // e11 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +Base testRParam2(Derived&& d) { + return d; // e12 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConstructFromDerived testRParam3(Derived&& d) { + return d; // e13 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConstructFromBase testRParam4(Derived&& d) { + return d; // e14 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConvertFromDerived testRParam5(Derived&& d) { + return d; // e15 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} +ConvertFromBase testRParam6(Derived&& d) { + return d; // e16 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:13}:"std::move(d)" +} + +// But if the return type is a reference type, then moving would be wrong. +Derived& testRetRef1(Derived&& d) { return d; } +Base& testRetRef2(Derived&& d) { return d; } +auto&& testRetRef3(Derived&& d) { return d; } +decltype(auto) testRetRef4(Derived&& d) { return (d); } + +// As long as we're checking parentheses, make sure parentheses don't disable the warning. +Base testParens1() { + Derived d; + return (d); // e17 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:15}:"std::move(d)" +} +ConstructFromDerived testParens2() { + Derived d; + return (d); // e18-cxx11 + // expected-warning@-1{{would have been copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:12-[[@LINE-3]]:15}:"std::move(d)" +} + + +// If the target is a catch-handler parameter, do apply the diagnostic. +void throw_derived(); +Derived testEParam1() { + try { throw_derived(); } catch (Derived d) { return d; } // e19 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)" + __builtin_unreachable(); +} +Base testEParam2() { + try { throw_derived(); } catch (Derived d) { return d; } // e20 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)" + __builtin_unreachable(); +} +ConstructFromDerived testEParam3() { + try { throw_derived(); } catch (Derived d) { return d; } // e21 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)" + __builtin_unreachable(); +} +ConstructFromBase testEParam4() { + try { throw_derived(); } catch (Derived d) { return d; } // e22 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)" + __builtin_unreachable(); +} +ConvertFromDerived testEParam5() { + try { throw_derived(); } catch (Derived d) { return d; } // e23 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)" + __builtin_unreachable(); +} +ConvertFromBase testEParam6() { + try { throw_derived(); } catch (Derived d) { return d; } // e24 + // expected-warning@-1{{will be copied despite being returned by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:57-[[@LINE-3]]:58}:"std::move(d)" + __builtin_unreachable(); +} + +// If the exception variable is an lvalue reference, we cannot be sure +// that we own it; it is extremely contrived, but possible, for this to +// be a reference to an exception object that was thrown via +// `std::rethrow_exception(xp)` in Thread A, and meanwhile somebody else +// has got a copy of `xp` in Thread B, so that moving out of this object +// in Thread A would be observable (and racy) with respect to Thread B. +// Therefore assume it's not safe to move from. +Derived ok_REParam1() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); } +Base ok_REParam2() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); } +ConstructFromDerived ok_REParam3() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); } +ConstructFromBase ok_REParam4() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); } +ConvertFromDerived ok_REParam5() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); } +ConvertFromBase ok_REParam6() { try { throw_derived(); } catch (Derived& d) { return d; } __builtin_unreachable(); } + +Derived ok_CEParam1() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); } +Base ok_CEParam2() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); } +ConstructFromDerived ok_CEParam3() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); } +ConstructFromBase ok_CEParam4() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); } +ConvertFromDerived ok_CEParam5() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); } +ConvertFromBase ok_CEParam6() { try { throw_derived(); } catch (const Derived& d) { return d; } __builtin_unreachable(); } + +// If rvalue overload resolution would find a copy constructor anyway, +// or if the copy constructor actually selected is trivial, then don't warn. +struct TriviallyCopyable {}; +struct OnlyCopyable { + OnlyCopyable() = default; + OnlyCopyable(const OnlyCopyable&) {} +}; + +TriviallyCopyable ok_copy1() { TriviallyCopyable c; return c; } +OnlyCopyable ok_copy2() { OnlyCopyable c; return c; } +TriviallyCopyable ok_copyparam1(TriviallyCopyable c) { return c; } +OnlyCopyable ok_copyparam2(OnlyCopyable c) { return c; } + +void test_throw1(Derived&& d) { + throw d; // e25 + // expected-warning@-1{{will be copied despite being thrown by name}} + // expected-note@-2{{to avoid copying}} + // CHECK: fix-it:"{{.*}}":{[[@LINE-3]]:11-[[@LINE-3]]:12}:"std::move(d)" +} + +void ok_throw1() { Derived d; throw d; } +void ok_throw2(Derived d) { throw d; } +void ok_throw3(Derived& d) { throw d; } +void ok_throw4(Derived d) { throw std::move(d); } +void ok_throw5(Derived& d) { throw std::move(d); } +void ok_throw6(Derived& d) { throw static_cast(d); } +void ok_throw7(TriviallyCopyable d) { throw d; } +void ok_throw8(OnlyCopyable d) { throw d; } -- 2.7.4