Quuxplusone updated this revision to Diff 139961.
Quuxplusone edited the summary of this revision.
Quuxplusone added a comment.

Rebased on master.  AFAIK this is ready to go and I just need someone to land 
it for me... @rtrieu ping?

I believe the PR summary would be suitable as a commit message.


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/SemaStmt.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/SemaStmt.cpp
===================================================================
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -2908,7 +2908,8 @@
   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;
@@ -2940,6 +2941,11 @@
 /// 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
@@ -2949,6 +2955,7 @@
                                   const VarDecl *NRVOCandidate,
                                   QualType ResultType,
                                   Expr *&Value,
+                                  bool ConvertingConstructorsOnly,
                                   ExprResult &Res) {
   ImplicitCastExpr AsRvalue(ImplicitCastExpr::OnStack, Value->getType(),
                             CK_NoOp, Value, VK_XValue);
@@ -2969,22 +2976,39 @@
       continue;
 
     FunctionDecl *FD = Step.Function.Function;
-    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;
+    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 {
-      continue;
+      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
@@ -3022,13 +3046,80 @@
   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 {
+        // 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);
+          }
+        }
+      }
     }
   }
 
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -3800,7 +3800,10 @@
     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,
Index: include/clang/Basic/DiagnosticSemaKinds.td
===================================================================
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -5619,6 +5619,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">,
@@ -723,7 +727,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
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to