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

[Jason Merrill]

On 3/5/19 4:53 PM, Marek Polacek wrote:
> 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?

OK.

> 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" }
> +}
>