[v2] c++: Extend -Wredundant-move for const-qual objects [PR90428]

Message ID YvFxnuEhiqRkdMeu@redhat.com
State New, archived
Headers
Series [v2] c++: Extend -Wredundant-move for const-qual objects [PR90428] |

Commit Message

Marek Polacek Aug. 8, 2022, 8:27 p.m. UTC
  On Sat, Aug 06, 2022 at 03:58:13PM -0800, Jason Merrill wrote:
> On 8/6/22 11:13, Marek Polacek wrote:
> > In this PR, Jon suggested extending the -Wredundant-move warning
> > to warn when the user is moving a const object as in:
> > 
> >    struct T { };
> > 
> >    T f(const T& t)
> >    {
> >      return std::move(t);
> >    }
> > 
> > where the std::move is redundant, because T does not have
> > a T(const T&&) constructor (which is very unlikely).  Even with
> > the std::move, T(T&&) would not be used because it would mean
> > losing the const.  Instead, T(const T&) will be called.
> > 
> > I had to restructure the function a bit, but it's better now.  This patch
> > depends on my other recent patches to maybe_warn_pessimizing_move.
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > 	PR c++/90428
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* typeck.cc (maybe_warn_pessimizing_move): Extend the
> > 	-Wredundant-move warning to warn about std::move on a
> > 	const-qualified object.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp0x/Wredundant-move1.C: Adjust dg-warning.
> > 	* g++.dg/cpp0x/Wredundant-move9.C: Likewise.
> > 	* g++.dg/cpp0x/Wredundant-move10.C: New test.
> > ---
> >   gcc/cp/typeck.cc                              | 157 +++++++++++-------
> >   gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C |   3 +-
> >   .../g++.dg/cpp0x/Wredundant-move10.C          |  61 +++++++
> >   gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C |   3 +-
> >   4 files changed, 162 insertions(+), 62 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
> > 
> > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> > index 70a5efc45de..802bc9c43fb 100644
> > --- a/gcc/cp/typeck.cc
> > +++ b/gcc/cp/typeck.cc
> > @@ -10411,72 +10411,109 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
> >   	return;
> >       }
> > -  /* We're looking for *std::move<T&> ((T &) &arg).  */
> > -  if (REFERENCE_REF_P (expr)
> > -      && TREE_CODE (TREE_OPERAND (expr, 0)) == CALL_EXPR)
> > -    {
> > -      tree fn = TREE_OPERAND (expr, 0);
> > -      if (is_std_move_p (fn))
> > -	{
> > -	  tree arg = CALL_EXPR_ARG (fn, 0);
> > -	  tree moved;
> > -	  if (TREE_CODE (arg) != NOP_EXPR)
> > -	    return;
> > -	  arg = TREE_OPERAND (arg, 0);
> > -	  if (TREE_CODE (arg) != ADDR_EXPR)
> > -	    return;
> > -	  arg = TREE_OPERAND (arg, 0);
> > -	  arg = convert_from_reference (arg);
> > -	  if (can_do_rvo_p (arg, type))
> > -	    {
> > -	      auto_diagnostic_group d;
> > -	      if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
> > -		  && warning_at (loc, OPT_Wpessimizing_move,
> > -				 "moving a temporary object prevents copy "
> > -				 "elision"))
> > -		inform (loc, "remove %<std::move%> call");
> > -	    }
> > -	  /* The rest of the warnings is only relevant for when we are
> > -	     returning from a function.  */
> > -	  else if (!return_p)
> > -	    return;
> > -	  /* Warn if we could do copy elision were it not for the move.  */
> > -	  else if (can_do_nrvo_p (arg, type))
> > +  /* First, check if this is a call to std::move.  */
> > +  if (!REFERENCE_REF_P (expr)
> > +      || TREE_CODE (TREE_OPERAND (expr, 0)) != CALL_EXPR)
> > +    return;
> > +  tree fn = TREE_OPERAND (expr, 0);
> > +  if (!is_std_move_p (fn))
> > +    return;
> > +  tree arg = CALL_EXPR_ARG (fn, 0);
> > +  if (TREE_CODE (arg) != NOP_EXPR)
> > +    return;
> > +  /* If we're looking at *std::move<T&> ((T &) &arg), do the pessimizing N/RVO
> > +     and implicitly-movable warnings.  */
> > +  if (TREE_CODE (TREE_OPERAND (arg, 0)) == ADDR_EXPR)
> > +    {
> > +      arg = TREE_OPERAND (arg, 0);
> > +      arg = TREE_OPERAND (arg, 0);
> > +      arg = convert_from_reference (arg);
> > +      if (can_do_rvo_p (arg, type))
> 
> Incidentally, this function should probably have a different name if we're
> checking it in non-return situations.

I've renamed it to can_elide_copy_prvalue_p in this patch.
 
> > +	{
> > +	  auto_diagnostic_group d;
> > +	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
> > +	      && warning_at (loc, OPT_Wpessimizing_move,
> > +			     "moving a temporary object prevents copy elision"))
> > +	    inform (loc, "remove %<std::move%> call");
> > +	}
> > +      /* The rest of the warnings is only relevant for when we are returning
> > +	 from a function.  */
> > +      if (!return_p)
> > +	return;
> > +
> > +      tree moved;
> > +      /* Warn if we could do copy elision were it not for the move.  */
> > +      if (can_do_nrvo_p (arg, type))
> > +	{
> > +	  auto_diagnostic_group d;
> > +	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
> > +	      && warning_at (loc, OPT_Wpessimizing_move,
> > +			     "moving a local object in a return statement "
> > +			     "prevents copy elision"))
> > +	    inform (loc, "remove %<std::move%> call");
> > +	}
> > +      /* Warn if the move is redundant.  It is redundant when we would
> > +	 do maybe-rvalue overload resolution even without std::move.  */
> > +      else if (warn_redundant_move
> > +	       && !warning_suppressed_p (expr, OPT_Wredundant_move)
> > +	       && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
> > +	{
> > +	  /* Make sure that overload resolution would actually succeed
> > +	     if we removed the std::move call.  */
> > +	  tree t = convert_for_initialization (NULL_TREE, type,
> > +					       moved,
> > +					       (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
> > +	      /* Trying to move something const will never succeed unless
> > +		 there's T(const T&&), which it almost never is, and if
> > +		 so, T wouldn't be error_mark_node now.  So do warn.  */
> > +	      || (CP_TYPE_CONST_P (TREE_TYPE (arg))
> > +		  && same_type_ignoring_top_level_qualifiers_p
> > +		  (TREE_TYPE (arg), type)))
> 
> I'm confused by the || case here.  Overload resolution fails if we drop the
> move call, but we're still going to warn?

This is to warn about this:

T f5(const T t)
{
  return std::move(t); // { dg-warning "redundant move" }
}

where OR fails because there's no T(const T&&) (or it's deleted in which case
convert_for_initialization also returns error_mark_node).  This OR is going to
fail with std::move but also without std::move when we're trying to treat an lvalue
as an rvalue.  So the std::move has no effect, because T(const T&) will be
called in either case.

Now, if there was a T(const T&&), we'd *still* warn, because the std::move would
still be redundant.  Does that make sense?

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

-- >8 --
In this PR, Jon suggested extending the -Wredundant-move warning
to warn when the user is moving a const object as in:

  struct T { };

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

where the std::move is redundant, because T does not have
a T(const T&&) constructor (which is very unlikely).  Even with
the std::move, T(T&&) would not be used because it would mean
losing the const.  Instead, T(const T&) will be called.

I had to restructure the function a bit, but it's better now.  This patch
depends on my other recent patches to maybe_warn_pessimizing_move.

	PR c++/90428

gcc/cp/ChangeLog:

	* typeck.cc (can_do_rvo_p): Rename to ...
	(can_elide_copy_prvalue_p): ... this.
	(maybe_warn_pessimizing_move): Extend the
	-Wredundant-move warning to warn about std::move on a
	const-qualified object.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp0x/Wredundant-move1.C: Adjust dg-warning.
	* g++.dg/cpp0x/Wredundant-move9.C: Likewise.
	* g++.dg/cpp0x/Wredundant-move10.C: New test.
---
 gcc/cp/typeck.cc                              | 157 +++++++++++-------
 gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C |   3 +-
 .../g++.dg/cpp0x/Wredundant-move10.C          |  61 +++++++
 gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C |   3 +-
 4 files changed, 162 insertions(+), 62 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C


base-commit: 053876cdbe8057210e6f4da4eec2df58f92ccd4c
prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5
prerequisite-patch-id: ce490c3c0b991fa7f27315387949c145c66223a4
prerequisite-patch-id: bf551f0fe75fa8f6775fe53469b61ae94d8cab81
  

Comments

Marek Polacek Aug. 9, 2022, 3:08 p.m. UTC | #1
On Mon, Aug 08, 2022 at 04:27:10PM -0400, Marek Polacek wrote:
> +  /* Also try to warn about redundant std::move in code such as
> +      T f (const T& t)
> +      {
> +	return std::move(t);
> +      }
> +    for which EXPR will be something like
> +      *std::move<const T&> ((const struct T &) (const struct T *) t)
> +     and where the std::move does nothing if T does not have a T(const T&&)
> +     constructor, because the argument is const.  It will not use T(T&&)
> +     because that would mean losing the const.  */
> +  else if (TREE_CODE (TREE_TYPE (arg)) == REFERENCE_TYPE

This is TYPE_REF_P so I'll fix that up.

> +	   && CP_TYPE_CONST_P (TREE_TYPE (TREE_TYPE (arg))))

Marek
  
Jason Merrill Aug. 15, 2022, 7:43 p.m. UTC | #2
On 8/8/22 13:27, Marek Polacek wrote:
> On Sat, Aug 06, 2022 at 03:58:13PM -0800, Jason Merrill wrote:
>> On 8/6/22 11:13, Marek Polacek wrote:
>>> In this PR, Jon suggested extending the -Wredundant-move warning
>>> to warn when the user is moving a const object as in:
>>>
>>>     struct T { };
>>>
>>>     T f(const T& t)
>>>     {
>>>       return std::move(t);
>>>     }
>>>
>>> where the std::move is redundant, because T does not have
>>> a T(const T&&) constructor (which is very unlikely).  Even with
>>> the std::move, T(T&&) would not be used because it would mean
>>> losing the const.  Instead, T(const T&) will be called.
>>>
>>> I had to restructure the function a bit, but it's better now.  This patch
>>> depends on my other recent patches to maybe_warn_pessimizing_move.
>>>
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>
>>> 	PR c++/90428
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* typeck.cc (maybe_warn_pessimizing_move): Extend the
>>> 	-Wredundant-move warning to warn about std::move on a
>>> 	const-qualified object.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/cpp0x/Wredundant-move1.C: Adjust dg-warning.
>>> 	* g++.dg/cpp0x/Wredundant-move9.C: Likewise.
>>> 	* g++.dg/cpp0x/Wredundant-move10.C: New test.
>>> ---
>>>    gcc/cp/typeck.cc                              | 157 +++++++++++-------
>>>    gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C |   3 +-
>>>    .../g++.dg/cpp0x/Wredundant-move10.C          |  61 +++++++
>>>    gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C |   3 +-
>>>    4 files changed, 162 insertions(+), 62 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
>>>
>>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
>>> index 70a5efc45de..802bc9c43fb 100644
>>> --- a/gcc/cp/typeck.cc
>>> +++ b/gcc/cp/typeck.cc
>>> @@ -10411,72 +10411,109 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>>>    	return;
>>>        }
>>> -  /* We're looking for *std::move<T&> ((T &) &arg).  */
>>> -  if (REFERENCE_REF_P (expr)
>>> -      && TREE_CODE (TREE_OPERAND (expr, 0)) == CALL_EXPR)
>>> -    {
>>> -      tree fn = TREE_OPERAND (expr, 0);
>>> -      if (is_std_move_p (fn))
>>> -	{
>>> -	  tree arg = CALL_EXPR_ARG (fn, 0);
>>> -	  tree moved;
>>> -	  if (TREE_CODE (arg) != NOP_EXPR)
>>> -	    return;
>>> -	  arg = TREE_OPERAND (arg, 0);
>>> -	  if (TREE_CODE (arg) != ADDR_EXPR)
>>> -	    return;
>>> -	  arg = TREE_OPERAND (arg, 0);
>>> -	  arg = convert_from_reference (arg);
>>> -	  if (can_do_rvo_p (arg, type))
>>> -	    {
>>> -	      auto_diagnostic_group d;
>>> -	      if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
>>> -		  && warning_at (loc, OPT_Wpessimizing_move,
>>> -				 "moving a temporary object prevents copy "
>>> -				 "elision"))
>>> -		inform (loc, "remove %<std::move%> call");
>>> -	    }
>>> -	  /* The rest of the warnings is only relevant for when we are
>>> -	     returning from a function.  */
>>> -	  else if (!return_p)
>>> -	    return;
>>> -	  /* Warn if we could do copy elision were it not for the move.  */
>>> -	  else if (can_do_nrvo_p (arg, type))
>>> +  /* First, check if this is a call to std::move.  */
>>> +  if (!REFERENCE_REF_P (expr)
>>> +      || TREE_CODE (TREE_OPERAND (expr, 0)) != CALL_EXPR)
>>> +    return;
>>> +  tree fn = TREE_OPERAND (expr, 0);
>>> +  if (!is_std_move_p (fn))
>>> +    return;
>>> +  tree arg = CALL_EXPR_ARG (fn, 0);
>>> +  if (TREE_CODE (arg) != NOP_EXPR)
>>> +    return;
>>> +  /* If we're looking at *std::move<T&> ((T &) &arg), do the pessimizing N/RVO
>>> +     and implicitly-movable warnings.  */
>>> +  if (TREE_CODE (TREE_OPERAND (arg, 0)) == ADDR_EXPR)
>>> +    {
>>> +      arg = TREE_OPERAND (arg, 0);
>>> +      arg = TREE_OPERAND (arg, 0);
>>> +      arg = convert_from_reference (arg);
>>> +      if (can_do_rvo_p (arg, type))
>>
>> Incidentally, this function should probably have a different name if we're
>> checking it in non-return situations.
> 
> I've renamed it to can_elide_copy_prvalue_p in this patch.
>   
>>> +	{
>>> +	  auto_diagnostic_group d;
>>> +	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
>>> +	      && warning_at (loc, OPT_Wpessimizing_move,
>>> +			     "moving a temporary object prevents copy elision"))
>>> +	    inform (loc, "remove %<std::move%> call");
>>> +	}
>>> +      /* The rest of the warnings is only relevant for when we are returning
>>> +	 from a function.  */
>>> +      if (!return_p)
>>> +	return;
>>> +
>>> +      tree moved;
>>> +      /* Warn if we could do copy elision were it not for the move.  */
>>> +      if (can_do_nrvo_p (arg, type))
>>> +	{
>>> +	  auto_diagnostic_group d;
>>> +	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
>>> +	      && warning_at (loc, OPT_Wpessimizing_move,
>>> +			     "moving a local object in a return statement "
>>> +			     "prevents copy elision"))
>>> +	    inform (loc, "remove %<std::move%> call");
>>> +	}
>>> +      /* Warn if the move is redundant.  It is redundant when we would
>>> +	 do maybe-rvalue overload resolution even without std::move.  */
>>> +      else if (warn_redundant_move
>>> +	       && !warning_suppressed_p (expr, OPT_Wredundant_move)
>>> +	       && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
>>> +	{
>>> +	  /* Make sure that overload resolution would actually succeed
>>> +	     if we removed the std::move call.  */
>>> +	  tree t = convert_for_initialization (NULL_TREE, type,
>>> +					       moved,
>>> +					       (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
>>> +	      /* Trying to move something const will never succeed unless
>>> +		 there's T(const T&&), which it almost never is, and if
>>> +		 so, T wouldn't be error_mark_node now.  So do warn.  */
>>> +	      || (CP_TYPE_CONST_P (TREE_TYPE (arg))
>>> +		  && same_type_ignoring_top_level_qualifiers_p
>>> +		  (TREE_TYPE (arg), type)))
>>
>> I'm confused by the || case here.  Overload resolution fails if we drop the
>> move call, but we're still going to warn?
> 
> This is to warn about this:
> 
> T f5(const T t)
> {
>    return std::move(t); // { dg-warning "redundant move" }
> }
> 
> where OR fails because there's no T(const T&&) (or it's deleted in which case
> convert_for_initialization also returns error_mark_node).  This OR is going to
> fail with std::move but also without std::move when we're trying to treat an lvalue
> as an rvalue.  So the std::move has no effect, because T(const T&) will be
> called in either case.
> 
> Now, if there was a T(const T&&), we'd *still* warn, because the std::move would
> still be redundant.  Does that make sense?

Ah, so this is because LOOKUP_PREFER_RVALUE returns an error if a const 
T& overload is selected.  The comment should mention that.  OK with that 
change.

Since you've been thinking about this area, I wonder if you want to look 
at implementing P2266?

> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> -- >8 --
> In this PR, Jon suggested extending the -Wredundant-move warning
> to warn when the user is moving a const object as in:
> 
>    struct T { };
> 
>    T f(const T& t)
>    {
>      return std::move(t);
>    }
> 
> where the std::move is redundant, because T does not have
> a T(const T&&) constructor (which is very unlikely).  Even with
> the std::move, T(T&&) would not be used because it would mean
> losing the const.  Instead, T(const T&) will be called.
> 
> I had to restructure the function a bit, but it's better now.  This patch
> depends on my other recent patches to maybe_warn_pessimizing_move.
> 
> 	PR c++/90428
> 
> gcc/cp/ChangeLog:
> 
> 	* typeck.cc (can_do_rvo_p): Rename to ...
> 	(can_elide_copy_prvalue_p): ... this.
> 	(maybe_warn_pessimizing_move): Extend the
> 	-Wredundant-move warning to warn about std::move on a
> 	const-qualified object.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/Wredundant-move1.C: Adjust dg-warning.
> 	* g++.dg/cpp0x/Wredundant-move9.C: Likewise.
> 	* g++.dg/cpp0x/Wredundant-move10.C: New test.
> ---
>   gcc/cp/typeck.cc                              | 157 +++++++++++-------
>   gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C |   3 +-
>   .../g++.dg/cpp0x/Wredundant-move10.C          |  61 +++++++
>   gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C |   3 +-
>   4 files changed, 162 insertions(+), 62 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
> 
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 70a5efc45de..5be79b83871 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10297,7 +10297,7 @@ can_do_nrvo_p (tree retval, tree functype)
>      prvalue.  */
>   
>   static bool
> -can_do_rvo_p (tree retval, tree functype)
> +can_elide_copy_prvalue_p (tree retval, tree functype)
>   {
>     if (functype == error_mark_node)
>       return false;
> @@ -10411,72 +10411,109 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>   	return;
>       }
>   
> -  /* We're looking for *std::move<T&> ((T &) &arg).  */
> -  if (REFERENCE_REF_P (expr)
> -      && TREE_CODE (TREE_OPERAND (expr, 0)) == CALL_EXPR)
> +  /* First, check if this is a call to std::move.  */
> +  if (!REFERENCE_REF_P (expr)
> +      || TREE_CODE (TREE_OPERAND (expr, 0)) != CALL_EXPR)
> +    return;
> +  tree fn = TREE_OPERAND (expr, 0);
> +  if (!is_std_move_p (fn))
> +    return;
> +  tree arg = CALL_EXPR_ARG (fn, 0);
> +  if (TREE_CODE (arg) != NOP_EXPR)
> +    return;
> +  /* If we're looking at *std::move<T&> ((T &) &arg), do the pessimizing N/RVO
> +     and implicitly-movable warnings.  */
> +  if (TREE_CODE (TREE_OPERAND (arg, 0)) == ADDR_EXPR)
>       {
> -      tree fn = TREE_OPERAND (expr, 0);
> -      if (is_std_move_p (fn))
> -	{
> -	  tree arg = CALL_EXPR_ARG (fn, 0);
> -	  tree moved;
> -	  if (TREE_CODE (arg) != NOP_EXPR)
> -	    return;
> -	  arg = TREE_OPERAND (arg, 0);
> -	  if (TREE_CODE (arg) != ADDR_EXPR)
> -	    return;
> -	  arg = TREE_OPERAND (arg, 0);
> -	  arg = convert_from_reference (arg);
> -	  if (can_do_rvo_p (arg, type))
> -	    {
> -	      auto_diagnostic_group d;
> -	      if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
> -		  && warning_at (loc, OPT_Wpessimizing_move,
> -				 "moving a temporary object prevents copy "
> -				 "elision"))
> -		inform (loc, "remove %<std::move%> call");
> -	    }
> -	  /* The rest of the warnings is only relevant for when we are
> -	     returning from a function.  */
> -	  else if (!return_p)
> -	    return;
> -	  /* Warn if we could do copy elision were it not for the move.  */
> -	  else if (can_do_nrvo_p (arg, type))
> +      arg = TREE_OPERAND (arg, 0);
> +      arg = TREE_OPERAND (arg, 0);
> +      arg = convert_from_reference (arg);
> +      if (can_elide_copy_prvalue_p (arg, type))
> +	{
> +	  auto_diagnostic_group d;
> +	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
> +	      && warning_at (loc, OPT_Wpessimizing_move,
> +			     "moving a temporary object prevents copy elision"))
> +	    inform (loc, "remove %<std::move%> call");
> +	}
> +      /* The rest of the warnings is only relevant for when we are returning
> +	 from a function.  */
> +      if (!return_p)
> +	return;
> +
> +      tree moved;
> +      /* Warn if we could do copy elision were it not for the move.  */
> +      if (can_do_nrvo_p (arg, type))
> +	{
> +	  auto_diagnostic_group d;
> +	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
> +	      && warning_at (loc, OPT_Wpessimizing_move,
> +			     "moving a local object in a return statement "
> +			     "prevents copy elision"))
> +	    inform (loc, "remove %<std::move%> call");
> +	}
> +      /* Warn if the move is redundant.  It is redundant when we would
> +	 do maybe-rvalue overload resolution even without std::move.  */
> +      else if (warn_redundant_move
> +	       && !warning_suppressed_p (expr, OPT_Wredundant_move)
> +	       && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
> +	{
> +	  /* Make sure that overload resolution would actually succeed
> +	     if we removed the std::move call.  */
> +	  tree t = convert_for_initialization (NULL_TREE, type,
> +					       moved,
> +					       (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
> +	      /* Trying to move something const will never succeed unless
> +		 there's T(const T&&), which it almost never is, and if
> +		 so, T wouldn't be error_mark_node now.  So do warn.  */
> +	      || (CP_TYPE_CONST_P (TREE_TYPE (arg))
> +		  && same_type_ignoring_top_level_qualifiers_p
> +		  (TREE_TYPE (arg), type)))
>   	    {
>   	      auto_diagnostic_group d;
> -	      if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
> -		  && warning_at (loc, OPT_Wpessimizing_move,
> -				 "moving a local object in a return statement "
> -				 "prevents copy elision"))
> +	      if (warning_at (loc, OPT_Wredundant_move,
> +			      "redundant move in return statement"))
>   		inform (loc, "remove %<std::move%> call");
>   	    }
> -	  /* Warn if the move is redundant.  It is redundant when we would
> -	     do maybe-rvalue overload resolution even without std::move.  */
> -	  else if (warn_redundant_move
> -		   && !warning_suppressed_p (expr, OPT_Wredundant_move)
> -		   && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
> -	    {
> -	      /* Make sure that overload resolution would actually succeed
> -		 if we removed the std::move call.  */
> -	      tree t = convert_for_initialization (NULL_TREE, type,
> -						   moved,
> -						   (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");
> -		}
> -	    }
>   	}
>       }
> +  /* Also try to warn about redundant std::move in code such as
> +      T f (const T& t)
> +      {
> +	return std::move(t);
> +      }
> +    for which EXPR will be something like
> +      *std::move<const T&> ((const struct T &) (const struct T *) t)
> +     and where the std::move does nothing if T does not have a T(const T&&)
> +     constructor, because the argument is const.  It will not use T(T&&)
> +     because that would mean losing the const.  */
> +  else if (TREE_CODE (TREE_TYPE (arg)) == REFERENCE_TYPE
> +	   && CP_TYPE_CONST_P (TREE_TYPE (TREE_TYPE (arg))))
> +    {
> +      tree rtype = TREE_TYPE (TREE_TYPE (arg));
> +      if (!same_type_ignoring_top_level_qualifiers_p (rtype, type))
> +	return;
> +      /* Check for the unlikely case there's T(const T&&) (we don't care if
> +	 it's deleted).  */
> +      for (tree fn : ovl_range (CLASSTYPE_CONSTRUCTORS (rtype)))
> +	if (move_fn_p (fn))
> +	  {
> +	    tree t = TREE_VALUE (FUNCTION_FIRST_USER_PARMTYPE (fn));
> +	    if (UNLIKELY (CP_TYPE_CONST_P (TREE_TYPE (t))))
> +	      return;
> +	  }
> +      auto_diagnostic_group d;
> +      if (warning_at (loc, OPT_Wredundant_move,
> +		      "redundant move in return statement"))
> +	inform (loc, "remove %<std::move%> call");
> +    }
>   }
>   
>   /* Check that returning RETVAL from the current function is valid.
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
> index ce4087b476f..c227019cce1 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
> @@ -60,7 +60,8 @@ fn4 (const T t)
>   {
>     // t is const: will decay into copy despite std::move, so it's redundant.
>     // We used to warn about this, but no longer since c++/87378.
> -  return std::move (t);	 // { dg-warning "redundant move" "" { target c++20 } }
> +  // Now we warn again since c++/90428.
> +  return std::move (t);	 // { dg-warning "redundant move" }
>   }
>   
>   int
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
> new file mode 100644
> index 00000000000..a215a4774d6
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
> @@ -0,0 +1,61 @@
> +// PR c++/90428
> +// { 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 T { T(); T(const T&); T(T&&) = delete; };
> +struct S : T { };
> +struct W { W(const W&); W(W&&) = delete; W(const W&&); };
> +
> +T f1(T t)
> +{
> +  const T& rt = t;
> +  return std::move(rt); // { dg-warning "redundant move" }
> +}
> +
> +T f2(const T& t)
> +{
> +  return std::move(t); // { dg-warning "redundant move" }
> +}
> +
> +W f3(const W& w)
> +{
> +  return std::move(w);
> +}
> +
> +T f4(const S& s)
> +{
> +  return std::move(s);
> +}
> +
> +T f5(const T t)
> +{
> +  return std::move(t); // { dg-warning "redundant move" }
> +}
> +
> +struct S1 { S1(S1 &&) = delete; S1(const S1&); };
> +struct S2: S1 {};
> +
> +S1 f3(const S2 s)
> +{
> +  return std::move(s); // { dg-warning "redundant move" "" { target c++20 } }
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C
> index ca1e23b7a4b..489ecd2b9c9 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C
> @@ -61,7 +61,8 @@ fn4 (const T<int> t)
>   {
>     // t is const: will decay into copy despite std::move, so it's redundant.
>     // We used to warn about this, but no longer since c++/87378.
> -  return std::move (t);	 // { dg-warning "redundant move" "" { target c++20 } }
> +  // Now we warn again since c++/90428.
> +  return std::move (t);	 // { dg-warning "redundant move" }
>   }
>   
>   int
> 
> base-commit: 053876cdbe8057210e6f4da4eec2df58f92ccd4c
> prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5
> prerequisite-patch-id: ce490c3c0b991fa7f27315387949c145c66223a4
> prerequisite-patch-id: bf551f0fe75fa8f6775fe53469b61ae94d8cab81
  
Marek Polacek Aug. 17, 2022, 7:09 p.m. UTC | #3
On Mon, Aug 15, 2022 at 03:43:38PM -0400, Jason Merrill wrote:
> On 8/8/22 13:27, Marek Polacek wrote:
> > This is to warn about this:
> > 
> > T f5(const T t)
> > {
> >    return std::move(t); // { dg-warning "redundant move" }
> > }
> > 
> > where OR fails because there's no T(const T&&) (or it's deleted in which case
> > convert_for_initialization also returns error_mark_node).  This OR is going to
> > fail with std::move but also without std::move when we're trying to treat an lvalue
> > as an rvalue.  So the std::move has no effect, because T(const T&) will be
> > called in either case.
> > 
> > Now, if there was a T(const T&&), we'd *still* warn, because the std::move would
> > still be redundant.  Does that make sense?
> 
> Ah, so this is because LOOKUP_PREFER_RVALUE returns an error if a const T&
> overload is selected.  The comment should mention that.  OK with that
> change.
 
Thanks, pushed.

> Since you've been thinking about this area, I wonder if you want to look at
> implementing P2266?

Yup, I can take a look.  It's also come up recently in a discussion with Davis
in https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89780#c3.

Since P2266 was approved, I've updated our C++23 status table as well.

Marek
  

Patch

diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 70a5efc45de..5be79b83871 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -10297,7 +10297,7 @@  can_do_nrvo_p (tree retval, tree functype)
    prvalue.  */
 
 static bool
-can_do_rvo_p (tree retval, tree functype)
+can_elide_copy_prvalue_p (tree retval, tree functype)
 {
   if (functype == error_mark_node)
     return false;
@@ -10411,72 +10411,109 @@  maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
 	return;
     }
 
-  /* We're looking for *std::move<T&> ((T &) &arg).  */
-  if (REFERENCE_REF_P (expr)
-      && TREE_CODE (TREE_OPERAND (expr, 0)) == CALL_EXPR)
+  /* First, check if this is a call to std::move.  */
+  if (!REFERENCE_REF_P (expr)
+      || TREE_CODE (TREE_OPERAND (expr, 0)) != CALL_EXPR)
+    return;
+  tree fn = TREE_OPERAND (expr, 0);
+  if (!is_std_move_p (fn))
+    return;
+  tree arg = CALL_EXPR_ARG (fn, 0);
+  if (TREE_CODE (arg) != NOP_EXPR)
+    return;
+  /* If we're looking at *std::move<T&> ((T &) &arg), do the pessimizing N/RVO
+     and implicitly-movable warnings.  */
+  if (TREE_CODE (TREE_OPERAND (arg, 0)) == ADDR_EXPR)
     {
-      tree fn = TREE_OPERAND (expr, 0);
-      if (is_std_move_p (fn))
-	{
-	  tree arg = CALL_EXPR_ARG (fn, 0);
-	  tree moved;
-	  if (TREE_CODE (arg) != NOP_EXPR)
-	    return;
-	  arg = TREE_OPERAND (arg, 0);
-	  if (TREE_CODE (arg) != ADDR_EXPR)
-	    return;
-	  arg = TREE_OPERAND (arg, 0);
-	  arg = convert_from_reference (arg);
-	  if (can_do_rvo_p (arg, type))
-	    {
-	      auto_diagnostic_group d;
-	      if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
-		  && warning_at (loc, OPT_Wpessimizing_move,
-				 "moving a temporary object prevents copy "
-				 "elision"))
-		inform (loc, "remove %<std::move%> call");
-	    }
-	  /* The rest of the warnings is only relevant for when we are
-	     returning from a function.  */
-	  else if (!return_p)
-	    return;
-	  /* Warn if we could do copy elision were it not for the move.  */
-	  else if (can_do_nrvo_p (arg, type))
+      arg = TREE_OPERAND (arg, 0);
+      arg = TREE_OPERAND (arg, 0);
+      arg = convert_from_reference (arg);
+      if (can_elide_copy_prvalue_p (arg, type))
+	{
+	  auto_diagnostic_group d;
+	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
+	      && warning_at (loc, OPT_Wpessimizing_move,
+			     "moving a temporary object prevents copy elision"))
+	    inform (loc, "remove %<std::move%> call");
+	}
+      /* The rest of the warnings is only relevant for when we are returning
+	 from a function.  */
+      if (!return_p)
+	return;
+
+      tree moved;
+      /* Warn if we could do copy elision were it not for the move.  */
+      if (can_do_nrvo_p (arg, type))
+	{
+	  auto_diagnostic_group d;
+	  if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
+	      && warning_at (loc, OPT_Wpessimizing_move,
+			     "moving a local object in a return statement "
+			     "prevents copy elision"))
+	    inform (loc, "remove %<std::move%> call");
+	}
+      /* Warn if the move is redundant.  It is redundant when we would
+	 do maybe-rvalue overload resolution even without std::move.  */
+      else if (warn_redundant_move
+	       && !warning_suppressed_p (expr, OPT_Wredundant_move)
+	       && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
+	{
+	  /* Make sure that overload resolution would actually succeed
+	     if we removed the std::move call.  */
+	  tree t = convert_for_initialization (NULL_TREE, type,
+					       moved,
+					       (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
+	      /* Trying to move something const will never succeed unless
+		 there's T(const T&&), which it almost never is, and if
+		 so, T wouldn't be error_mark_node now.  So do warn.  */
+	      || (CP_TYPE_CONST_P (TREE_TYPE (arg))
+		  && same_type_ignoring_top_level_qualifiers_p
+		  (TREE_TYPE (arg), type)))
 	    {
 	      auto_diagnostic_group d;
-	      if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
-		  && warning_at (loc, OPT_Wpessimizing_move,
-				 "moving a local object in a return statement "
-				 "prevents copy elision"))
+	      if (warning_at (loc, OPT_Wredundant_move,
+			      "redundant move in return statement"))
 		inform (loc, "remove %<std::move%> call");
 	    }
-	  /* Warn if the move is redundant.  It is redundant when we would
-	     do maybe-rvalue overload resolution even without std::move.  */
-	  else if (warn_redundant_move
-		   && !warning_suppressed_p (expr, OPT_Wredundant_move)
-		   && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
-	    {
-	      /* Make sure that overload resolution would actually succeed
-		 if we removed the std::move call.  */
-	      tree t = convert_for_initialization (NULL_TREE, type,
-						   moved,
-						   (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");
-		}
-	    }
 	}
     }
+  /* Also try to warn about redundant std::move in code such as
+      T f (const T& t)
+      {
+	return std::move(t);
+      }
+    for which EXPR will be something like
+      *std::move<const T&> ((const struct T &) (const struct T *) t)
+     and where the std::move does nothing if T does not have a T(const T&&)
+     constructor, because the argument is const.  It will not use T(T&&)
+     because that would mean losing the const.  */
+  else if (TREE_CODE (TREE_TYPE (arg)) == REFERENCE_TYPE
+	   && CP_TYPE_CONST_P (TREE_TYPE (TREE_TYPE (arg))))
+    {
+      tree rtype = TREE_TYPE (TREE_TYPE (arg));
+      if (!same_type_ignoring_top_level_qualifiers_p (rtype, type))
+	return;
+      /* Check for the unlikely case there's T(const T&&) (we don't care if
+	 it's deleted).  */
+      for (tree fn : ovl_range (CLASSTYPE_CONSTRUCTORS (rtype)))
+	if (move_fn_p (fn))
+	  {
+	    tree t = TREE_VALUE (FUNCTION_FIRST_USER_PARMTYPE (fn));
+	    if (UNLIKELY (CP_TYPE_CONST_P (TREE_TYPE (t))))
+	      return;
+	  }
+      auto_diagnostic_group d;
+      if (warning_at (loc, OPT_Wredundant_move,
+		      "redundant move in return statement"))
+	inform (loc, "remove %<std::move%> call");
+    }
 }
 
 /* Check that returning RETVAL from the current function is valid.
diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
index ce4087b476f..c227019cce1 100644
--- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
+++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
@@ -60,7 +60,8 @@  fn4 (const T t)
 {
   // t is const: will decay into copy despite std::move, so it's redundant.
   // We used to warn about this, but no longer since c++/87378.
-  return std::move (t);	 // { dg-warning "redundant move" "" { target c++20 } }
+  // Now we warn again since c++/90428.
+  return std::move (t);	 // { dg-warning "redundant move" }
 }
 
 int
diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
new file mode 100644
index 00000000000..a215a4774d6
--- /dev/null
+++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move10.C
@@ -0,0 +1,61 @@ 
+// PR c++/90428
+// { 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 T { T(); T(const T&); T(T&&) = delete; };
+struct S : T { };
+struct W { W(const W&); W(W&&) = delete; W(const W&&); };
+
+T f1(T t)
+{
+  const T& rt = t;
+  return std::move(rt); // { dg-warning "redundant move" }
+}
+
+T f2(const T& t)
+{
+  return std::move(t); // { dg-warning "redundant move" }
+}
+
+W f3(const W& w)
+{
+  return std::move(w);
+}
+
+T f4(const S& s)
+{
+  return std::move(s);
+}
+
+T f5(const T t)
+{
+  return std::move(t); // { dg-warning "redundant move" }
+}
+
+struct S1 { S1(S1 &&) = delete; S1(const S1&); };
+struct S2: S1 {};
+
+S1 f3(const S2 s)
+{
+  return std::move(s); // { dg-warning "redundant move" "" { target c++20 } }
+}
diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C
index ca1e23b7a4b..489ecd2b9c9 100644
--- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C
+++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move9.C
@@ -61,7 +61,8 @@  fn4 (const T<int> t)
 {
   // t is const: will decay into copy despite std::move, so it's redundant.
   // We used to warn about this, but no longer since c++/87378.
-  return std::move (t);	 // { dg-warning "redundant move" "" { target c++20 } }
+  // Now we warn again since c++/90428.
+  return std::move (t);	 // { dg-warning "redundant move" }
 }
 
 int