c++: Extend -Wpessimizing-move to other contexts

  In my recent patch which enhanced -Wpessimizing-move so that it warns
about class prvalues too I said that I'd like to extend it so that it
warns in more contexts where a std::move can prevent copy elision, such
as:

  T t = std::move(T());
  T t(std::move(T()));
  T t{std::move(T())};
  T t = {std::move(T())};
  void foo (T);
  foo (std::move(T()));

This patch does that by adding two maybe_warn_pessimizing_move calls.
These must happen before we've converted the initializers otherwise the
std::move will be buried in a TARGET_EXPR.

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

	PR c++/106276

gcc/cp/ChangeLog:

	* call.cc (build_over_call): Call maybe_warn_pessimizing_move.
	* cp-tree.h (maybe_warn_pessimizing_move): Declare.
	* decl.cc (build_aggr_init_full_exprs): Call
	maybe_warn_pessimizing_move.
	* typeck.cc (maybe_warn_pessimizing_move): Handle TREE_LIST and
	CONSTRUCTOR.  Add a bool parameter and use it.  Adjust a diagnostic
	message.
	(check_return_expr): Adjust the call to maybe_warn_pessimizing_move.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp0x/Wpessimizing-move7.C: Add dg-warning.
	* g++.dg/cpp0x/Wpessimizing-move8.C: New test.
---
 gcc/cp/call.cc                                |  5 +-
 gcc/cp/cp-tree.h                              |  1 +
 gcc/cp/decl.cc                                |  3 +-
 gcc/cp/typeck.cc                              | 58 ++++++++++++-----
 .../g++.dg/cpp0x/Wpessimizing-move7.C         | 16 ++---
 .../g++.dg/cpp0x/Wpessimizing-move8.C         | 65 +++++++++++++++++++
 6 files changed, 120 insertions(+), 28 deletions(-)
 create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C


base-commit: 502605a277d36cee1b0570982a16d97a43eace67
prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5
  

Ping.  (The other std::move patches depend on this one.)

(can_do_rvo_p is renamed to can_elide_copy_prvalue_p in the PR90428 patch.)

On Tue, Aug 02, 2022 at 07:04:47PM -0400, Marek Polacek via Gcc-patches wrote:
> In my recent patch which enhanced -Wpessimizing-move so that it warns
> about class prvalues too I said that I'd like to extend it so that it
> warns in more contexts where a std::move can prevent copy elision, such
> as:
> 
>   T t = std::move(T());
>   T t(std::move(T()));
>   T t{std::move(T())};
>   T t = {std::move(T())};
>   void foo (T);
>   foo (std::move(T()));
> 
> This patch does that by adding two maybe_warn_pessimizing_move calls.
> These must happen before we've converted the initializers otherwise the
> std::move will be buried in a TARGET_EXPR.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> 	PR c++/106276
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (build_over_call): Call maybe_warn_pessimizing_move.
> 	* cp-tree.h (maybe_warn_pessimizing_move): Declare.
> 	* decl.cc (build_aggr_init_full_exprs): Call
> 	maybe_warn_pessimizing_move.
> 	* typeck.cc (maybe_warn_pessimizing_move): Handle TREE_LIST and
> 	CONSTRUCTOR.  Add a bool parameter and use it.  Adjust a diagnostic
> 	message.
> 	(check_return_expr): Adjust the call to maybe_warn_pessimizing_move.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/Wpessimizing-move7.C: Add dg-warning.
> 	* g++.dg/cpp0x/Wpessimizing-move8.C: New test.
> ---
>  gcc/cp/call.cc                                |  5 +-
>  gcc/cp/cp-tree.h                              |  1 +
>  gcc/cp/decl.cc                                |  3 +-
>  gcc/cp/typeck.cc                              | 58 ++++++++++++-----
>  .../g++.dg/cpp0x/Wpessimizing-move7.C         | 16 ++---
>  .../g++.dg/cpp0x/Wpessimizing-move8.C         | 65 +++++++++++++++++++
>  6 files changed, 120 insertions(+), 28 deletions(-)
>  create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index 01a7be10077..370137ebd6d 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -9627,10 +9627,13 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
>        if (!conversion_warning)
>  	arg_complain &= ~tf_warning;
>  
> +      if (arg_complain & tf_warning)
> +	maybe_warn_pessimizing_move (arg, type, /*return_p*/false);
> +
>        val = convert_like_with_context (conv, arg, fn, i - is_method,
>  				       arg_complain);
>        val = convert_for_arg_passing (type, val, arg_complain);
> -	
> +
>        if (val == error_mark_node)
>          return error_mark_node;
>        else
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 3278b4114bd..5a8af22b509 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -8101,6 +8101,7 @@ extern tree finish_right_unary_fold_expr     (tree, int);
>  extern tree finish_binary_fold_expr          (tree, tree, int);
>  extern tree treat_lvalue_as_rvalue_p	     (tree, bool);
>  extern bool decl_in_std_namespace_p	     (tree);
> +extern void maybe_warn_pessimizing_move	     (tree, tree, bool);
>  
>  /* in typeck2.cc */
>  extern void require_complete_eh_spec_types	(tree, tree);
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 70ad681467e..dc6853a7de1 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -7220,9 +7220,10 @@ check_array_initializer (tree decl, tree type, tree init)
>  
>  static tree
>  build_aggr_init_full_exprs (tree decl, tree init, int flags)
> -     
>  {
>    gcc_assert (stmts_are_full_exprs_p ());
> +  if (init)
> +    maybe_warn_pessimizing_move (init, TREE_TYPE (decl), /*return_p*/false);
>    return build_aggr_init (decl, init, flags, tf_warning_or_error);
>  }
>  
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 9500c4e2fe8..2650beb780e 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10368,17 +10368,17 @@ treat_lvalue_as_rvalue_p (tree expr, bool return_p)
>      }
>  }
>  
> -/* Warn about wrong usage of std::move in a return statement.  RETVAL
> -   is the expression we are returning; FUNCTYPE is the type the function
> -   is declared to return.  */
> +/* Warn about dubious usage of std::move (in a return statement, if RETURN_P
> +   is true).  EXPR is the std::move expression; TYPE is the type of the object
> +   being initialized.  */
>  
> -static void
> -maybe_warn_pessimizing_move (tree retval, tree functype)
> +void
> +maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>  {
>    if (!(warn_pessimizing_move || warn_redundant_move))
>      return;
>  
> -  location_t loc = cp_expr_loc_or_input_loc (retval);
> +  const location_t loc = cp_expr_loc_or_input_loc (expr);
>  
>    /* C++98 doesn't know move.  */
>    if (cxx_dialect < cxx11)
> @@ -10390,14 +10390,32 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>      return;
>  
>    /* This is only interesting for class types.  */
> -  if (!CLASS_TYPE_P (functype))
> +  if (!CLASS_TYPE_P (type))
>      return;
>  
> +  /* A a = std::move (A());  */
> +  if (TREE_CODE (expr) == TREE_LIST)
> +    {
> +      if (list_length (expr) == 1)
> +	expr = TREE_VALUE (expr);
> +      else
> +	return;
> +    }
> +  /* A a = {std::move (A())};
> +     A a{std::move (A())};  */
> +  else if (TREE_CODE (expr) == CONSTRUCTOR)
> +    {
> +      if (CONSTRUCTOR_NELTS (expr) == 1)
> +	expr = CONSTRUCTOR_ELT (expr, 0)->value;
> +      else
> +	return;
> +    }
> +
>    /* We're looking for *std::move<T&> ((T &) &arg).  */
> -  if (REFERENCE_REF_P (retval)
> -      && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR)
> +  if (REFERENCE_REF_P (expr)
> +      && TREE_CODE (TREE_OPERAND (expr, 0)) == CALL_EXPR)
>      {
> -      tree fn = TREE_OPERAND (retval, 0);
> +      tree fn = TREE_OPERAND (expr, 0);
>        if (is_std_move_p (fn))
>  	{
>  	  tree arg = CALL_EXPR_ARG (fn, 0);
> @@ -10409,20 +10427,24 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>  	    return;
>  	  arg = TREE_OPERAND (arg, 0);
>  	  arg = convert_from_reference (arg);
> -	  /* Warn if we could do copy elision were it not for the move.  */
> -	  if (can_do_nrvo_p (arg, functype))
> +	  if (can_do_rvo_p (arg, type))
>  	    {
>  	      auto_diagnostic_group d;
>  	      if (warning_at (loc, OPT_Wpessimizing_move,
> -			      "moving a local object in a return statement "
> -			      "prevents copy elision"))
> +			      "moving a temporary object prevents copy "
> +			      "elision"))
>  		inform (loc, "remove %<std::move%> call");
>  	    }
> -	  else if (can_do_rvo_p (arg, functype))
> +	  /* 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))
>  	    {
>  	      auto_diagnostic_group d;
>  	      if (warning_at (loc, OPT_Wpessimizing_move,
> -			      "moving a temporary object in a return statement "
> +			      "moving a local object in a return statement "
>  			      "prevents copy elision"))
>  		inform (loc, "remove %<std::move%> call");
>  	    }
> @@ -10433,7 +10455,7 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>  	    {
>  	      /* Make sure that overload resolution would actually succeed
>  		 if we removed the std::move call.  */
> -	      tree t = convert_for_initialization (NULL_TREE, functype,
> +	      tree t = convert_for_initialization (NULL_TREE, type,
>  						   moved,
>  						   (LOOKUP_NORMAL
>  						    | LOOKUP_ONLYCONVERTING
> @@ -10718,7 +10740,7 @@ check_return_expr (tree retval, bool *no_warning)
>      return NULL_TREE;
>  
>    if (!named_return_value_okay_p)
> -    maybe_warn_pessimizing_move (retval, functype);
> +    maybe_warn_pessimizing_move (retval, functype, /*return_p*/true);
>  
>    /* Do any required conversions.  */
>    if (bare_retval == result || DECL_CONSTRUCTOR_P (current_function_decl))
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> index cd4eaa09aae..a17c7a87b7d 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> @@ -30,23 +30,23 @@ static A foo ();
>  A
>  fn1 ()
>  {
> -  return std::move (A{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> -  return std::move (A()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> -  return std::move (foo ()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  return std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (foo ()); // { dg-warning "moving a temporary object prevents copy elision" }
>  }
>  
>  B fn2 ()
>  {
> -  return std::move (A());
> -  return std::move (A{});
> -  return std::move (foo ());
> +  return std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (foo ()); // { dg-warning "moving a temporary object prevents copy elision" }
>  }
>  
>  template <typename T1, typename T2>
>  T1
>  fn3 ()
>  {
> -  return std::move (T2{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" }
>  }
>  
>  void
> @@ -58,6 +58,6 @@ do_fn3 ()
>  
>  char take_buffer;
>  struct label_text {
> -  label_text take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  label_text take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object prevents copy elision" }
>    label_text(char *);
>  };
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
> new file mode 100644
> index 00000000000..51406c8f97f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
> @@ -0,0 +1,65 @@
> +// PR c++/106276
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wpessimizing-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 A { A(); A(const A&) = delete; A(A&&); };
> +struct B { B(A); };
> +struct X { };
> +
> +void foo (A);
> +void bar (X);
> +
> +void
> +fn1 ()
> +{
> +  A a1 = std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a2 = std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a3(std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a4(std::move (A{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a5{std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a6{std::move (A{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a7 = {std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a8 = {std::move (A{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  B b1 = std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  B b2(std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  B b3{std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  B b4 = {std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  X x1 = std::move (X()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x2 = std::move (X{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x3(std::move (X())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x4(std::move (X{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x5{std::move (X())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x6{std::move (X{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x7 = {std::move (X())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x8 = {std::move (X{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  foo (std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  foo (std::move (A{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +  bar (std::move (X())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  bar (std::move (X{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  foo (std::move (a1));
> +  bar (std::move (x1));
> +}
> 
> base-commit: 502605a277d36cee1b0570982a16d97a43eace67
> prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5
> -- 
> 2.37.1
> 

Marek
  

On 8/2/22 16:04, Marek Polacek wrote:
> In my recent patch which enhanced -Wpessimizing-move so that it warns
> about class prvalues too I said that I'd like to extend it so that it
> warns in more contexts where a std::move can prevent copy elision, such
> as:
> 
>    T t = std::move(T());
>    T t(std::move(T()));
>    T t{std::move(T())};
>    T t = {std::move(T())};
>    void foo (T);
>    foo (std::move(T()));
> 
> This patch does that by adding two maybe_warn_pessimizing_move calls.
> These must happen before we've converted the initializers otherwise the
> std::move will be buried in a TARGET_EXPR.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> 
> 	PR c++/106276
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (build_over_call): Call maybe_warn_pessimizing_move.
> 	* cp-tree.h (maybe_warn_pessimizing_move): Declare.
> 	* decl.cc (build_aggr_init_full_exprs): Call
> 	maybe_warn_pessimizing_move.
> 	* typeck.cc (maybe_warn_pessimizing_move): Handle TREE_LIST and
> 	CONSTRUCTOR.  Add a bool parameter and use it.  Adjust a diagnostic
> 	message.
> 	(check_return_expr): Adjust the call to maybe_warn_pessimizing_move.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/Wpessimizing-move7.C: Add dg-warning.
> 	* g++.dg/cpp0x/Wpessimizing-move8.C: New test.
> ---
>   gcc/cp/call.cc                                |  5 +-
>   gcc/cp/cp-tree.h                              |  1 +
>   gcc/cp/decl.cc                                |  3 +-
>   gcc/cp/typeck.cc                              | 58 ++++++++++++-----
>   .../g++.dg/cpp0x/Wpessimizing-move7.C         | 16 ++---
>   .../g++.dg/cpp0x/Wpessimizing-move8.C         | 65 +++++++++++++++++++
>   6 files changed, 120 insertions(+), 28 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index 01a7be10077..370137ebd6d 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -9627,10 +9627,13 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
>         if (!conversion_warning)
>   	arg_complain &= ~tf_warning;
>   
> +      if (arg_complain & tf_warning)
> +	maybe_warn_pessimizing_move (arg, type, /*return_p*/false);
> +
>         val = convert_like_with_context (conv, arg, fn, i - is_method,
>   				       arg_complain);
>         val = convert_for_arg_passing (type, val, arg_complain);
> -	
> +
>         if (val == error_mark_node)
>           return error_mark_node;
>         else
> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> index 3278b4114bd..5a8af22b509 100644
> --- a/gcc/cp/cp-tree.h
> +++ b/gcc/cp/cp-tree.h
> @@ -8101,6 +8101,7 @@ extern tree finish_right_unary_fold_expr     (tree, int);
>   extern tree finish_binary_fold_expr          (tree, tree, int);
>   extern tree treat_lvalue_as_rvalue_p	     (tree, bool);
>   extern bool decl_in_std_namespace_p	     (tree);
> +extern void maybe_warn_pessimizing_move	     (tree, tree, bool);
>   
>   /* in typeck2.cc */
>   extern void require_complete_eh_spec_types	(tree, tree);
> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> index 70ad681467e..dc6853a7de1 100644
> --- a/gcc/cp/decl.cc
> +++ b/gcc/cp/decl.cc
> @@ -7220,9 +7220,10 @@ check_array_initializer (tree decl, tree type, tree init)
>   
>   static tree
>   build_aggr_init_full_exprs (tree decl, tree init, int flags)
> -
>   {
>     gcc_assert (stmts_are_full_exprs_p ());
> +  if (init)
> +    maybe_warn_pessimizing_move (init, TREE_TYPE (decl), /*return_p*/false);

This is a surprising place to add this.  Why here rather than in 
build_aggr_init or check_initializer?

>     return build_aggr_init (decl, init, flags, tf_warning_or_error);
>   }
>   
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 9500c4e2fe8..2650beb780e 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10368,17 +10368,17 @@ treat_lvalue_as_rvalue_p (tree expr, bool return_p)
>       }
>   }
>   
> -/* Warn about wrong usage of std::move in a return statement.  RETVAL
> -   is the expression we are returning; FUNCTYPE is the type the function
> -   is declared to return.  */
> +/* Warn about dubious usage of std::move (in a return statement, if RETURN_P
> +   is true).  EXPR is the std::move expression; TYPE is the type of the object
> +   being initialized.  */
>   
> -static void
> -maybe_warn_pessimizing_move (tree retval, tree functype)
> +void
> +maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>   {
>     if (!(warn_pessimizing_move || warn_redundant_move))
>       return;
>   
> -  location_t loc = cp_expr_loc_or_input_loc (retval);
> +  const location_t loc = cp_expr_loc_or_input_loc (expr);
>   
>     /* C++98 doesn't know move.  */
>     if (cxx_dialect < cxx11)
> @@ -10390,14 +10390,32 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>       return;
>   
>     /* This is only interesting for class types.  */
> -  if (!CLASS_TYPE_P (functype))
> +  if (!CLASS_TYPE_P (type))
>       return;
>   
> +  /* A a = std::move (A());  */
> +  if (TREE_CODE (expr) == TREE_LIST)
> +    {
> +      if (list_length (expr) == 1)
> +	expr = TREE_VALUE (expr);
> +      else
> +	return;
> +    }
> +  /* A a = {std::move (A())};
> +     A a{std::move (A())};  */
> +  else if (TREE_CODE (expr) == CONSTRUCTOR)
> +    {
> +      if (CONSTRUCTOR_NELTS (expr) == 1)
> +	expr = CONSTRUCTOR_ELT (expr, 0)->value;
> +      else
> +	return;
> +    }
> +
>     /* We're looking for *std::move<T&> ((T &) &arg).  */
> -  if (REFERENCE_REF_P (retval)
> -      && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR)
> +  if (REFERENCE_REF_P (expr)
> +      && TREE_CODE (TREE_OPERAND (expr, 0)) == CALL_EXPR)
>       {
> -      tree fn = TREE_OPERAND (retval, 0);
> +      tree fn = TREE_OPERAND (expr, 0);
>         if (is_std_move_p (fn))
>   	{
>   	  tree arg = CALL_EXPR_ARG (fn, 0);
> @@ -10409,20 +10427,24 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>   	    return;
>   	  arg = TREE_OPERAND (arg, 0);
>   	  arg = convert_from_reference (arg);
> -	  /* Warn if we could do copy elision were it not for the move.  */
> -	  if (can_do_nrvo_p (arg, functype))
> +	  if (can_do_rvo_p (arg, type))
>   	    {
>   	      auto_diagnostic_group d;
>   	      if (warning_at (loc, OPT_Wpessimizing_move,
> -			      "moving a local object in a return statement "
> -			      "prevents copy elision"))
> +			      "moving a temporary object prevents copy "
> +			      "elision"))
>   		inform (loc, "remove %<std::move%> call");
>   	    }
> -	  else if (can_do_rvo_p (arg, functype))
> +	  /* 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))
>   	    {
>   	      auto_diagnostic_group d;
>   	      if (warning_at (loc, OPT_Wpessimizing_move,
> -			      "moving a temporary object in a return statement "
> +			      "moving a local object in a return statement "
>   			      "prevents copy elision"))
>   		inform (loc, "remove %<std::move%> call");
>   	    }
> @@ -10433,7 +10455,7 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>   	    {
>   	      /* Make sure that overload resolution would actually succeed
>   		 if we removed the std::move call.  */
> -	      tree t = convert_for_initialization (NULL_TREE, functype,
> +	      tree t = convert_for_initialization (NULL_TREE, type,
>   						   moved,
>   						   (LOOKUP_NORMAL
>   						    | LOOKUP_ONLYCONVERTING
> @@ -10718,7 +10740,7 @@ check_return_expr (tree retval, bool *no_warning)
>       return NULL_TREE;
>   
>     if (!named_return_value_okay_p)
> -    maybe_warn_pessimizing_move (retval, functype);
> +    maybe_warn_pessimizing_move (retval, functype, /*return_p*/true);
>   
>     /* Do any required conversions.  */
>     if (bare_retval == result || DECL_CONSTRUCTOR_P (current_function_decl))
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> index cd4eaa09aae..a17c7a87b7d 100644
> --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
> @@ -30,23 +30,23 @@ static A foo ();
>   A
>   fn1 ()
>   {
> -  return std::move (A{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> -  return std::move (A()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> -  return std::move (foo ()); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  return std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (foo ()); // { dg-warning "moving a temporary object prevents copy elision" }
>   }
>   
>   B fn2 ()
>   {
> -  return std::move (A());
> -  return std::move (A{});
> -  return std::move (foo ());
> +  return std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  return std::move (foo ()); // { dg-warning "moving a temporary object prevents copy elision" }
>   }
>   
>   template <typename T1, typename T2>
>   T1
>   fn3 ()
>   {
> -  return std::move (T2{}); // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" }
>   }
>   
>   void
> @@ -58,6 +58,6 @@ do_fn3 ()
>   
>   char take_buffer;
>   struct label_text {
> -  label_text take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object in a return statement prevents copy elision" }
> +  label_text take() { return std::move(label_text(&take_buffer)); } // { dg-warning "moving a temporary object prevents copy elision" }
>     label_text(char *);
>   };
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
> new file mode 100644
> index 00000000000..51406c8f97f
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
> @@ -0,0 +1,65 @@
> +// PR c++/106276
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wpessimizing-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 A { A(); A(const A&) = delete; A(A&&); };
> +struct B { B(A); };
> +struct X { };
> +
> +void foo (A);
> +void bar (X);
> +
> +void
> +fn1 ()
> +{
> +  A a1 = std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a2 = std::move (A{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a3(std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a4(std::move (A{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a5{std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a6{std::move (A{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a7 = {std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  A a8 = {std::move (A{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  B b1 = std::move (A()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  B b2(std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  B b3{std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  B b4 = {std::move (A())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  X x1 = std::move (X()); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x2 = std::move (X{}); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x3(std::move (X())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x4(std::move (X{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x5{std::move (X())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x6{std::move (X{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x7 = {std::move (X())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +  X x8 = {std::move (X{})}; // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  foo (std::move (A())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  foo (std::move (A{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +  bar (std::move (X())); // { dg-warning "moving a temporary object prevents copy elision" }
> +  bar (std::move (X{})); // { dg-warning "moving a temporary object prevents copy elision" }
> +
> +  foo (std::move (a1));
> +  bar (std::move (x1));
> +}
> 
> base-commit: 502605a277d36cee1b0570982a16d97a43eace67
> prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5
  

On Tue, Aug 16, 2022 at 03:23:18PM -0400, Jason Merrill wrote:
> On 8/2/22 16:04, Marek Polacek wrote:
> > In my recent patch which enhanced -Wpessimizing-move so that it warns
> > about class prvalues too I said that I'd like to extend it so that it
> > warns in more contexts where a std::move can prevent copy elision, such
> > as:
> > 
> >    T t = std::move(T());
> >    T t(std::move(T()));
> >    T t{std::move(T())};
> >    T t = {std::move(T())};
> >    void foo (T);
> >    foo (std::move(T()));
> > 
> > This patch does that by adding two maybe_warn_pessimizing_move calls.
> > These must happen before we've converted the initializers otherwise the
> > std::move will be buried in a TARGET_EXPR.
> > 
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> > 
> > 	PR c++/106276
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* call.cc (build_over_call): Call maybe_warn_pessimizing_move.
> > 	* cp-tree.h (maybe_warn_pessimizing_move): Declare.
> > 	* decl.cc (build_aggr_init_full_exprs): Call
> > 	maybe_warn_pessimizing_move.
> > 	* typeck.cc (maybe_warn_pessimizing_move): Handle TREE_LIST and
> > 	CONSTRUCTOR.  Add a bool parameter and use it.  Adjust a diagnostic
> > 	message.
> > 	(check_return_expr): Adjust the call to maybe_warn_pessimizing_move.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/cpp0x/Wpessimizing-move7.C: Add dg-warning.
> > 	* g++.dg/cpp0x/Wpessimizing-move8.C: New test.
> > ---
> >   gcc/cp/call.cc                                |  5 +-
> >   gcc/cp/cp-tree.h                              |  1 +
> >   gcc/cp/decl.cc                                |  3 +-
> >   gcc/cp/typeck.cc                              | 58 ++++++++++++-----
> >   .../g++.dg/cpp0x/Wpessimizing-move7.C         | 16 ++---
> >   .../g++.dg/cpp0x/Wpessimizing-move8.C         | 65 +++++++++++++++++++
> >   6 files changed, 120 insertions(+), 28 deletions(-)
> >   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
> > 
> > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > index 01a7be10077..370137ebd6d 100644
> > --- a/gcc/cp/call.cc
> > +++ b/gcc/cp/call.cc
> > @@ -9627,10 +9627,13 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
> >         if (!conversion_warning)
> >   	arg_complain &= ~tf_warning;
> > +      if (arg_complain & tf_warning)
> > +	maybe_warn_pessimizing_move (arg, type, /*return_p*/false);
> > +
> >         val = convert_like_with_context (conv, arg, fn, i - is_method,
> >   				       arg_complain);
> >         val = convert_for_arg_passing (type, val, arg_complain);
> > -	
> > +
> >         if (val == error_mark_node)
> >           return error_mark_node;
> >         else
> > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
> > index 3278b4114bd..5a8af22b509 100644
> > --- a/gcc/cp/cp-tree.h
> > +++ b/gcc/cp/cp-tree.h
> > @@ -8101,6 +8101,7 @@ extern tree finish_right_unary_fold_expr     (tree, int);
> >   extern tree finish_binary_fold_expr          (tree, tree, int);
> >   extern tree treat_lvalue_as_rvalue_p	     (tree, bool);
> >   extern bool decl_in_std_namespace_p	     (tree);
> > +extern void maybe_warn_pessimizing_move	     (tree, tree, bool);
> >   /* in typeck2.cc */
> >   extern void require_complete_eh_spec_types	(tree, tree);
> > diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
> > index 70ad681467e..dc6853a7de1 100644
> > --- a/gcc/cp/decl.cc
> > +++ b/gcc/cp/decl.cc
> > @@ -7220,9 +7220,10 @@ check_array_initializer (tree decl, tree type, tree init)
> >   static tree
> >   build_aggr_init_full_exprs (tree decl, tree init, int flags)
> > -
> >   {
> >     gcc_assert (stmts_are_full_exprs_p ());
> > +  if (init)
> > +    maybe_warn_pessimizing_move (init, TREE_TYPE (decl), /*return_p*/false);
> 
> This is a surprising place to add this.  Why here rather than in
> build_aggr_init or check_initializer?

IIRC it just felt appropriate since we only want to invoke maybe_warn_ on the
full expression, not any subexpressions -- we're looking to see if the
outermost expr is a std::move.  Also, we want to warn for all types, not just
classes.

But I can move the call into some place in check_initializer if you prefer.

Marek
  

On 8/16/22 14:09, Marek Polacek wrote:
> On Tue, Aug 16, 2022 at 03:23:18PM -0400, Jason Merrill wrote:
>> On 8/2/22 16:04, Marek Polacek wrote:
>>> In my recent patch which enhanced -Wpessimizing-move so that it warns
>>> about class prvalues too I said that I'd like to extend it so that it
>>> warns in more contexts where a std::move can prevent copy elision, such
>>> as:
>>>
>>>     T t = std::move(T());
>>>     T t(std::move(T()));
>>>     T t{std::move(T())};
>>>     T t = {std::move(T())};
>>>     void foo (T);
>>>     foo (std::move(T()));
>>>
>>> This patch does that by adding two maybe_warn_pessimizing_move calls.
>>> These must happen before we've converted the initializers otherwise the
>>> std::move will be buried in a TARGET_EXPR.
>>>
>>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>>
>>> 	PR c++/106276
>>>
>>> gcc/cp/ChangeLog:
>>>
>>> 	* call.cc (build_over_call): Call maybe_warn_pessimizing_move.
>>> 	* cp-tree.h (maybe_warn_pessimizing_move): Declare.
>>> 	* decl.cc (build_aggr_init_full_exprs): Call
>>> 	maybe_warn_pessimizing_move.
>>> 	* typeck.cc (maybe_warn_pessimizing_move): Handle TREE_LIST and
>>> 	CONSTRUCTOR.  Add a bool parameter and use it.  Adjust a diagnostic
>>> 	message.
>>> 	(check_return_expr): Adjust the call to maybe_warn_pessimizing_move.
>>>
>>> gcc/testsuite/ChangeLog:
>>>
>>> 	* g++.dg/cpp0x/Wpessimizing-move7.C: Add dg-warning.
>>> 	* g++.dg/cpp0x/Wpessimizing-move8.C: New test.
>>> ---
>>>    gcc/cp/call.cc                                |  5 +-
>>>    gcc/cp/cp-tree.h                              |  1 +
>>>    gcc/cp/decl.cc                                |  3 +-
>>>    gcc/cp/typeck.cc                              | 58 ++++++++++++-----
>>>    .../g++.dg/cpp0x/Wpessimizing-move7.C         | 16 ++---
>>>    .../g++.dg/cpp0x/Wpessimizing-move8.C         | 65 +++++++++++++++++++
>>>    6 files changed, 120 insertions(+), 28 deletions(-)
>>>    create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move8.C
>>>
>>> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
>>> index 01a7be10077..370137ebd6d 100644
>>> --- a/gcc/cp/call.cc
>>> +++ b/gcc/cp/call.cc
>>> @@ -9627,10 +9627,13 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
>>>          if (!conversion_warning)
>>>    	arg_complain &= ~tf_warning;
>>> +      if (arg_complain & tf_warning)
>>> +	maybe_warn_pessimizing_move (arg, type, /*return_p*/false);
>>> +
>>>          val = convert_like_with_context (conv, arg, fn, i - is_method,
>>>    				       arg_complain);
>>>          val = convert_for_arg_passing (type, val, arg_complain);
>>> -	
>>> +
>>>          if (val == error_mark_node)
>>>            return error_mark_node;
>>>          else
>>> diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h
>>> index 3278b4114bd..5a8af22b509 100644
>>> --- a/gcc/cp/cp-tree.h
>>> +++ b/gcc/cp/cp-tree.h
>>> @@ -8101,6 +8101,7 @@ extern tree finish_right_unary_fold_expr     (tree, int);
>>>    extern tree finish_binary_fold_expr          (tree, tree, int);
>>>    extern tree treat_lvalue_as_rvalue_p	     (tree, bool);
>>>    extern bool decl_in_std_namespace_p	     (tree);
>>> +extern void maybe_warn_pessimizing_move	     (tree, tree, bool);
>>>    /* in typeck2.cc */
>>>    extern void require_complete_eh_spec_types	(tree, tree);
>>> diff --git a/gcc/cp/decl.cc b/gcc/cp/decl.cc
>>> index 70ad681467e..dc6853a7de1 100644
>>> --- a/gcc/cp/decl.cc
>>> +++ b/gcc/cp/decl.cc
>>> @@ -7220,9 +7220,10 @@ check_array_initializer (tree decl, tree type, tree init)
>>>    static tree
>>>    build_aggr_init_full_exprs (tree decl, tree init, int flags)
>>> -
>>>    {
>>>      gcc_assert (stmts_are_full_exprs_p ());
>>> +  if (init)
>>> +    maybe_warn_pessimizing_move (init, TREE_TYPE (decl), /*return_p*/false);
>>
>> This is a surprising place to add this.  Why here rather than in
>> build_aggr_init or check_initializer?
> 
> IIRC it just felt appropriate since we only want to invoke maybe_warn_ on the
> full expression, not any subexpressions -- we're looking to see if the
> outermost expr is a std::move.  Also, we want to warn for all types, not just
> classes.

Makes sense.  The patch is OK.

> But I can move the call into some place in check_initializer if you prefer.
> 
> Marek
>
  

On 16/08/2022 14:27, Marek Polacek via Gcc-patches wrote:
> Ping.  (The other std::move patches depend on this one.)

<https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=8d22c7cb8b1a6f9b67c54a798dd5504244614e51> 
"c++: Extend -Wpessimizing-move to other contexts" started to cause 
false positive

> $ cat test.cc
> #include <utility>
> struct S1 {
>     S1();
>     S1(S1 const &) = delete;
>     S1(S1 &&);
>     S1 operator =(S1 const &) = delete;
>     S1 operator =(S1 &&);
> };
> struct S2 { S2(S1); };
> S2 f() {
>     S1 s;
>     return { std::move(s) };
> }
> 
> $ g++ -fsyntax-only -Wredundant-move test.cc
> test.cc: In function ‘S2 f()’:
> test.cc:12:27: warning: redundant move in return statement [-Wredundant-move]
>    12 |     return { std::move(s) };
>       |                           ^
> test.cc:12:27: note: remove ‘std::move’ call
  

On Mon, Aug 22, 2022 at 01:48:34PM +0200, Stephan Bergmann wrote:
> On 16/08/2022 14:27, Marek Polacek via Gcc-patches wrote:
> > Ping.  (The other std::move patches depend on this one.)
> 
> <https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=8d22c7cb8b1a6f9b67c54a798dd5504244614e51>
> "c++: Extend -Wpessimizing-move to other contexts" started to cause false
> positive

Thanks for reporting the problem.  I'm testing a simple patch (appended
below) and will post it once testing finishes.
 
> > $ cat test.cc
> > #include <utility>
> > struct S1 {
> >     S1();
> >     S1(S1 const &) = delete;
> >     S1(S1 &&);
> >     S1 operator =(S1 const &) = delete;
> >     S1 operator =(S1 &&);
> > };
> > struct S2 { S2(S1); };
> > S2 f() {
> >     S1 s;
> >     return { std::move(s) };
> > }
> > 
> > $ g++ -fsyntax-only -Wredundant-move test.cc
> > test.cc: In function ‘S2 f()’:
> > test.cc:12:27: warning: redundant move in return statement [-Wredundant-move]
> >    12 |     return { std::move(s) };
> >       |                           ^
> > test.cc:12:27: note: remove ‘std::move’ call

--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -10447,7 +10447,7 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
     return;

   /* A a = std::move (A());  */
-  if (TREE_CODE (expr) == TREE_LIST)
+  if (TREE_CODE (expr) == TREE_LIST && !return_p)
     {
       if (list_length (expr) == 1)
    expr = TREE_VALUE (expr);
@@ -10456,7 +10456,7 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
     }
   /* A a = {std::move (A())};
      A a{std::move (A())};  */
-  else if (TREE_CODE (expr) == CONSTRUCTOR)
+  else if (TREE_CODE (expr) == CONSTRUCTOR && !return_p)
     {
       if (CONSTRUCTOR_NELTS (expr) == 1)
    expr = CONSTRUCTOR_ELT (expr, 0)->value;

Marek