C++ PATCH for c++/87378 - bogus -Wredundant-move warning

[Marek Polacek]

On Tue, Mar 05, 2019 at 03:50:30PM -0500, Jason Merrill wrote:
> On 3/4/19 7:17 PM, Marek Polacek wrote:
> > This patch fixes a bogus -Wredundant-move warning.  In the test in the PR
> > the std::move call isn't redundant; the testcase won't actually compile
> > without that call, as per the resolution of bug 87150.
> > 
> > Before this patch, we'd issue the redundant-move warning anytime
> > treat_lvalue_as_rvalue_p is true for a std::move's argument.  But we also
> > need to make sure that convert_for_initialization works even without the
> > std::move call, if not, it's not redundant.
> 
> Indeed.
> 
> > Trouble arises when the argument is const.  Then it might be the case that
> > the implicit rvalue fails because it uses a const copy constructor, or
> > that the type of the returned object and the type of the selected ctor's
> > parameter aren't the same.
> 
> So this is the case where std::move is redundant because doing overload
> resolution on the lvalue would select the same constructor?  I'm not sure
> that's worth warning about, especially in templates where we don't know
> anything about the return type.

Yes, it's about:

struct T {
  T(const T&);
  T(T&&);
};

T
f(const T t)
{
  return t; // uses const T&
  return std::move (t); // also uses const T&
}

I'm fine with dropping the warning in this (IMO fairly obscure) case; I
certainly didn't have this in mind when adding the warning.

So the patch can be simplified to the following:

Bootstrapped/regtested on x86_64-linux, ok for trunk?

2019-03-05  Marek Polacek  <polacek@redhat.com>

	PR c++/87378 - bogus -Wredundant-move warning.
	* typeck.c (maybe_warn_pessimizing_move): See if the maybe-rvalue
	overload resolution would actually succeed.

	* g++.dg/cpp0x/Wredundant-move1.C (fn4): Drop dg-warning.
	* g++.dg/cpp0x/Wredundant-move7.C: New test.

diff --git gcc/cp/typeck.c gcc/cp/typeck.c
index 1bf9ad88141..43ff3d63abd 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9429,10 +9429,24 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
 	     do maybe-rvalue overload resolution even without std::move.  */
 	  else if (treat_lvalue_as_rvalue_p (arg, /*parm_ok*/true))
 	    {
-	      auto_diagnostic_group d;
-	      if (warning_at (loc, OPT_Wredundant_move,
-			      "redundant move in return statement"))
-		inform (loc, "remove %<std::move%> call");
+	      /* Make sure that the overload resolution would actually succeed
+		 if we removed the std::move call.  */
+	      tree t = convert_for_initialization (NULL_TREE, functype,
+						   move (arg),
+						   (LOOKUP_NORMAL
+						    | LOOKUP_ONLYCONVERTING
+						    | LOOKUP_PREFER_RVALUE),
+						   ICR_RETURN, NULL_TREE, 0,
+						   tf_none);
+	      /* If this worked, implicit rvalue would work, so the call to
+		 std::move is redundant.  */
+	      if (t != error_mark_node)
+		{
+		  auto_diagnostic_group d;
+		  if (warning_at (loc, OPT_Wredundant_move,
+				  "redundant move in return statement"))
+		    inform (loc, "remove %<std::move%> call");
+		}
 	    }
 	}
     }
diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
index 5d4a25dbc3b..e70f3cde625 100644
--- gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
+++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
@@ -59,7 +59,8 @@ T
 fn4 (const T t)
 {
   // t is const: will decay into copy despite std::move, so it's redundant.
-  return std::move (t); // { dg-warning "redundant move in return statement" }
+  // We used to warn about this, but no longer since c++/87378.
+  return std::move (t);
 }
 
 int
diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C
new file mode 100644
index 00000000000..015d7c4f7a4
--- /dev/null
+++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move7.C
@@ -0,0 +1,59 @@
+// PR c++/87378
+// { dg-do compile { target c++11 } }
+// { dg-options "-Wredundant-move" }
+
+// Define std::move.
+namespace std {
+  template<typename _Tp>
+    struct remove_reference
+    { typedef _Tp   type; };
+
+  template<typename _Tp>
+    struct remove_reference<_Tp&>
+    { typedef _Tp   type; };
+
+  template<typename _Tp>
+    struct remove_reference<_Tp&&>
+    { typedef _Tp   type; };
+
+  template<typename _Tp>
+    constexpr typename std::remove_reference<_Tp>::type&&
+    move(_Tp&& __t) noexcept
+    { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
+}
+
+struct S1 { S1(S1 &&); };
+struct S2 : S1 {};
+
+S1
+f (S2 s)
+{
+  return std::move(s); // { dg-bogus "redundant move in return statement" }
+}
+
+struct R1 {
+  R1(R1 &&);
+  R1(const R1 &&);
+};
+struct R2 : R1 {};
+
+R1
+f2 (const R2 s)
+{
+  return std::move(s); // { dg-bogus "redundant move in return statement" }
+}
+
+struct T1 {
+  T1(const T1 &);
+  T1(T1 &&);
+  T1(const T1 &&);
+};
+struct T2 : T1 {};
+
+T1
+f3 (const T2 s)
+{
+  // Without std::move: const T1 &
+  // With std::move: const T1 &&
+  return std::move(s); // { dg-bogus "redundant move in return statement" }
+}