Quuxplusone updated this revision to Diff 138457.
Quuxplusone added a comment.
Boldly add `-Wreturn-std-move` to `-Wmove` (and thus also to `-Wall`).
The backward-compatibility-concerned diagnostic, `-Wreturn-std-move-in-c++11`,
is *not* appropriate for `-Wmove`; but I believe this main diagnostic is
appropriate.
Repository:
rC Clang
https://reviews.llvm.org/D43322
Files:
include/clang/Basic/DiagnosticGroups.td
include/clang/Basic/DiagnosticSemaKinds.td
include/clang/Sema/Sema.h
lib/Sema/SemaExprCXX.cpp
lib/Sema/SemaStmt.cpp
lib/Sema/SemaTemplateInstantiateDecl.cpp
test/SemaCXX/warn-return-std-move.cpp
Index: test/SemaCXX/warn-return-std-move.cpp
===================================================================
--- /dev/null
+++ test/SemaCXX/warn-return-std-move.cpp
@@ -0,0 +1,334 @@
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wreturn-std-move -Wreturn-std-move-in-cxx11 -std=c++11 -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+// definitions for std::move
+namespace std {
+inline namespace foo {
+template <class T> struct remove_reference { typedef T type; };
+template <class T> struct remove_reference<T&> { typedef T type; };
+template <class T> struct remove_reference<T&&> { typedef T type; };
+
+template <class T> typename remove_reference<T>::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<Derived&&>(d); }
+ConstructFromDerived ok3() { Derived d; return static_cast<Derived&&>(d); }
+ConstructFromBase ok4() { Derived d; return static_cast<Derived&&>(d); }
+ConvertFromDerived ok5() { Derived d; return static_cast<Derived&&>(d); }
+ConvertFromBase ok6() { Derived d; return static_cast<Derived&&>(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<Derived&&>(d); }
+void ok_throw7(TriviallyCopyable d) { throw d; }
+void ok_throw8(OnlyCopyable d) { throw d; }
Index: lib/Sema/SemaTemplateInstantiateDecl.cpp
===================================================================
--- lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -742,7 +742,7 @@
if (D->isNRVOVariable()) {
QualType ReturnType = cast<FunctionDecl>(DC)->getReturnType();
- if (SemaRef.isCopyElisionCandidate(ReturnType, Var, false))
+ if (SemaRef.isCopyElisionCandidate(ReturnType, Var, Sema::CES_Strict))
Var->setNRVOVariable(true);
}
Index: lib/Sema/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2862,16 +2862,16 @@
/// \param E The expression being returned from the function or block, or
/// being thrown.
///
-/// \param AllowParamOrMoveConstructible Whether we allow function parameters or
+/// \param CESK Whether we allow function parameters or
/// id-expressions that could be moved out of the function to be considered NRVO
/// candidates. C++ prohibits these for NRVO itself, but we re-use this logic to
/// determine whether we should try to move as part of a return or throw (which
/// does allow function parameters).
///
/// \returns The NRVO candidate variable, if the return statement may use the
/// NRVO, or NULL if there is no such candidate.
VarDecl *Sema::getCopyElisionCandidate(QualType ReturnType, Expr *E,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
if (!getLangOpts().CPlusPlus)
return nullptr;
@@ -2884,31 +2884,32 @@
if (!VD)
return nullptr;
- if (isCopyElisionCandidate(ReturnType, VD, AllowParamOrMoveConstructible))
+ if (isCopyElisionCandidate(ReturnType, VD, CESK))
return VD;
return nullptr;
}
bool Sema::isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
- bool AllowParamOrMoveConstructible) {
+ CopyElisionSemanticsKind CESK) {
QualType VDType = VD->getType();
// - in a return statement in a function with ...
// ... a class return type ...
if (!ReturnType.isNull() && !ReturnType->isDependentType()) {
if (!ReturnType->isRecordType())
return false;
// ... the same cv-unqualified type as the function return type ...
// When considering moving this expression out, allow dissimilar types.
- if (!AllowParamOrMoveConstructible && !VDType->isDependentType() &&
+ if (!(CESK & CES_AllowDifferentTypes) && !VDType->isDependentType() &&
!Context.hasSameUnqualifiedType(ReturnType, VDType))
return false;
}
// ...object (other than a function or catch-clause parameter)...
if (VD->getKind() != Decl::Var &&
- !(AllowParamOrMoveConstructible && VD->getKind() == Decl::ParmVar))
+ !((CESK & CES_AllowParameters) && VD->getKind() == Decl::ParmVar))
+ return false;
+ if (!(CESK & CES_AllowExceptionVariables) && VD->isExceptionVariable())
return false;
- if (VD->isExceptionVariable()) return false;
// ...automatic...
if (!VD->hasLocalStorage()) return false;
@@ -2918,7 +2919,7 @@
// variable will no longer be used.
if (VD->hasAttr<BlocksAttr>()) return false;
- if (AllowParamOrMoveConstructible)
+ if (CESK & CES_AllowDifferentTypes)
return true;
// ...non-volatile...
@@ -2933,6 +2934,94 @@
return true;
}
+/// \brief Try to perform the initialization of a potentially-movable value,
+/// which is the operand to a return or throw statement.
+///
+/// This routine implements C++14 [class.copy]p32, which attempts to treat
+/// 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
+/// invalid state.
+static void TryMoveInitialization(Sema& S,
+ const InitializedEntity &Entity,
+ const VarDecl *NRVOCandidate,
+ QualType ResultType,
+ Expr *&Value,
+ bool ConvertingConstructorsOnly,
+ ExprResult &Res) {
+ ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
+ CK_NoOp, Value, VK_XValue);
+
+ Expr *InitExpr = &AsRvalue;
+
+ InitializationKind Kind = InitializationKind::CreateCopy(
+ Value->getLocStart(), Value->getLocStart());
+
+ InitializationSequence Seq(S, Entity, Kind, InitExpr);
+
+ if (!Seq)
+ return;
+
+ for (const InitializationSequence::Step &Step : Seq.steps()) {
+ if (Step.Kind != InitializationSequence::SK_ConstructorInitialization &&
+ Step.Kind != InitializationSequence::SK_UserConversion)
+ continue;
+
+ FunctionDecl *FD = Step.Function.Function;
+ if (ConvertingConstructorsOnly) {
+ if (isa<CXXConstructorDecl>(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<RValueReferenceType>();
+ if (!RRefType)
+ break;
+ if (!S.Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
+ NRVOCandidate->getType()))
+ break;
+ } else {
+ continue;
+ }
+ } else {
+ if (isa<CXXConstructorDecl>(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<RValueReferenceType>(FD->getParamDecl(0)->getType()))
+ break;
+ } else if (isa<CXXMethodDecl>(FD)) {
+ // Check that overload resolution selected a conversion operator
+ // taking an rvalue reference.
+ if (cast<CXXMethodDecl>(FD)->getRefQualifier() != RQ_RValue)
+ break;
+ } else {
+ continue;
+ }
+ }
+
+ // Promote "AsRvalue" to the heap, since we now need this
+ // expression node to persist.
+ Value = ImplicitCastExpr::Create(S.Context, Value->getType(), CK_NoOp,
+ Value, nullptr, VK_XValue);
+
+ // Complete type-checking the initialization of the return type
+ // using the constructor we found.
+ Res = Seq.Perform(S, Entity, Kind, Value);
+ }
+}
+
/// \brief Perform the initialization of a potentially-movable value, which
/// is the result of return value.
///
@@ -2956,52 +3045,80 @@
// were designated by an rvalue.
ExprResult Res = ExprError();
- if (AllowNRVO && !NRVOCandidate)
- NRVOCandidate = getCopyElisionCandidate(ResultType, Value, true);
-
- if (AllowNRVO && NRVOCandidate) {
- ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
- CK_NoOp, Value, VK_XValue);
-
- Expr *InitExpr = &AsRvalue;
-
- InitializationKind Kind = InitializationKind::CreateCopy(
- Value->getLocStart(), Value->getLocStart());
-
- InitializationSequence Seq(*this, Entity, Kind, InitExpr);
- if (Seq) {
- for (const InitializationSequence::Step &Step : Seq.steps()) {
- if (!(Step.Kind ==
- InitializationSequence::SK_ConstructorInitialization ||
- (Step.Kind == InitializationSequence::SK_UserConversion &&
- isa<CXXConstructorDecl>(Step.Function.Function))))
- continue;
-
- CXXConstructorDecl *Constructor =
- cast<CXXConstructorDecl>(Step.Function.Function);
-
- const RValueReferenceType *RRefType
- = Constructor->getParamDecl(0)->getType()
- ->getAs<RValueReferenceType>();
-
- // [...] 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.
- if (!RRefType ||
- !Context.hasSameUnqualifiedType(RRefType->getPointeeType(),
- NRVOCandidate->getType()))
- break;
+ 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);
+ }
+ }
- // Promote "AsRvalue" to the heap, since we now need this
- // expression node to persist.
- Value = ImplicitCastExpr::Create(Context, Value->getType(), CK_NoOp,
- Value, nullptr, VK_XValue);
+ if (NRVOCandidate) {
+ TryMoveInitialization(*this, Entity, NRVOCandidate, ResultType, Value,
+ true, Res);
+ }
- // Complete type-checking the initialization of the return type
- // using the constructor we found.
- Res = Seq.Perform(*this, Entity, Kind, Value);
+ 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 {
+ // The most common case for this is returning T from a function that
+ // returns Expected<T>. This is totally fine in a post-CWG1579 world,
+ // but was not fine before.
+ assert(ResultType);
+ 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);
+ }
+ }
}
}
}
@@ -3149,7 +3266,7 @@
// In C++ the return statement is handled via a copy initialization.
// the C version of which boils down to CheckSingleAssignmentConstraints.
- NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false);
+ NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, CES_Strict);
InitializedEntity Entity = InitializedEntity::InitializeResult(ReturnLoc,
FnRetType,
NRVOCandidate != nullptr);
@@ -3162,7 +3279,7 @@
RetValExp = Res.get();
CheckReturnValExpr(RetValExp, FnRetType, ReturnLoc);
} else {
- NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false);
+ NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, CES_Strict);
}
if (RetValExp) {
@@ -3532,7 +3649,7 @@
// In C++ the return statement is handled via a copy initialization,
// the C version of which boils down to CheckSingleAssignmentConstraints.
if (RetValExp)
- NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, false);
+ NRVOCandidate = getCopyElisionCandidate(FnRetType, RetValExp, CES_Strict);
if (!HasDependentReturnType && !RetValExp->isTypeDependent()) {
// we have a non-void function with an expression, continue checking
InitializedEntity Entity = InitializedEntity::InitializeResult(ReturnLoc,
Index: lib/Sema/SemaExprCXX.cpp
===================================================================
--- lib/Sema/SemaExprCXX.cpp
+++ lib/Sema/SemaExprCXX.cpp
@@ -728,7 +728,7 @@
// exception object
const VarDecl *NRVOVariable = nullptr;
if (IsThrownVarInScope)
- NRVOVariable = getCopyElisionCandidate(QualType(), Ex, false);
+ NRVOVariable = getCopyElisionCandidate(QualType(), Ex, CES_Strict);
InitializedEntity Entity = InitializedEntity::InitializeException(
OpLoc, ExceptionObjectTy,
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -3795,10 +3795,21 @@
RecordDecl *CreateCapturedStmtRecordDecl(CapturedDecl *&CD,
SourceLocation Loc,
unsigned NumParams);
+
+ enum CopyElisionSemanticsKind {
+ 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,
- bool AllowParamOrMoveConstructible);
+ CopyElisionSemanticsKind CESK);
bool isCopyElisionCandidate(QualType ReturnType, const VarDecl *VD,
- bool AllowParamOrMoveConstructible);
+ CopyElisionSemanticsKind CESK);
StmtResult ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
Scope *CurScope);
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5638,6 +5638,19 @@
InGroup<PessimizingMove>, 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<ReturnStdMove>, 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<ReturnStdMoveInCXX11>, 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<StringPlusInt>;
Index: include/clang/Basic/DiagnosticGroups.td
===================================================================
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -383,7 +383,11 @@
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">,
@@ -718,7 +722,12 @@
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,
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
