Re: [PATCH] c++: Tweaks for -Wredundant-move [PR107363]

[Jason Merrill <jason@redhat.com>]

On 10/28/22 10:42, Marek Polacek wrote:
> Two things here:
> 
> 1) when we're pointing out that std::move on a constant object is
>     redundant, don't say "in return statement" when we aren't in a
>     return statement;
> 2) suppress the warning when the std::move call was dependent, because
>     removing the std::move may not be correct for a different
>     instantiation of the original template.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

OK.  I'd probably do #2 with a warning_sentinel in 
tsubst_copy_and_build/CALL_EXPR, but this way makes sense, too.

> 	PR c++/107363
> 
> gcc/cp/ChangeLog:
> 
> 	* semantics.cc (finish_call_expr): Suppress OPT_Wpessimizing_move.
> 	* typeck.cc (maybe_warn_pessimizing_move): Check warn_redundant_move
> 	and warning_suppressed_p.  Adjust a message depending on return_p.
> 	(check_return_expr): Don't suppress OPT_Wpessimizing_move here.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/Wredundant-move13.C: New test.
> ---
>   gcc/cp/semantics.cc                           |  4 ++
>   gcc/cp/typeck.cc                              | 16 ++---
>   .../g++.dg/cpp0x/Wredundant-move13.C          | 61 +++++++++++++++++++
>   3 files changed, 73 insertions(+), 8 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wredundant-move13.C
> 
> diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
> index 36aa9c4499f..caaa40fde19 100644
> --- a/gcc/cp/semantics.cc
> +++ b/gcc/cp/semantics.cc
> @@ -2738,6 +2738,10 @@ finish_call_expr (tree fn, vec<tree, va_gc> **args, bool disallow_virtual,
>   	  result = build_min_nt_call_vec (orig_fn, *args);
>   	  SET_EXPR_LOCATION (result, cp_expr_loc_or_input_loc (fn));
>   	  KOENIG_LOOKUP_P (result) = koenig_p;
> +	  /* Disable the std::move warnings since this call was dependent
> +	     (c++/89780, c++/107363).  This also suppresses the
> +	     -Wredundant-move warning.  */
> +	  suppress_warning (result, OPT_Wpessimizing_move);
>   	  if (is_overloaded_fn (fn))
>   	    fn = get_fns (fn);
>   
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 2e0fd8fbf17..5f5fb2a212b 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10885,7 +10885,9 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>        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 (TYPE_REF_P (TREE_TYPE (arg))
> +  else if (warn_redundant_move
> +	   && !warning_suppressed_p (expr, OPT_Wredundant_move)
> +	   && TYPE_REF_P (TREE_TYPE (arg))
>   	   && CP_TYPE_CONST_P (TREE_TYPE (TREE_TYPE (arg))))
>       {
>         tree rtype = TREE_TYPE (TREE_TYPE (arg));
> @@ -10901,8 +10903,11 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>   	      return;
>   	  }
>         auto_diagnostic_group d;
> -      if (warning_at (loc, OPT_Wredundant_move,
> -		      "redundant move in return statement"))
> +      if (return_p
> +	  ? warning_at (loc, OPT_Wredundant_move,
> +			"redundant move in return statement")
> +	  : warning_at (loc, OPT_Wredundant_move,
> +			"redundant move in initialization"))
>   	inform (loc, "remove %<std::move%> call");
>       }
>   }
> @@ -11126,11 +11131,6 @@ check_return_expr (tree retval, bool *no_warning)
>         /* We don't know if this is an lvalue or rvalue use, but
>   	 either way we can mark it as read.  */
>         mark_exp_read (retval);
> -      /* Disable our std::move warnings when we're returning
> -	 a dependent expression (c++/89780).  */
> -      if (retval && TREE_CODE (retval) == CALL_EXPR)
> -	/* This also suppresses -Wredundant-move.  */
> -	suppress_warning (retval, OPT_Wpessimizing_move);
>         return retval;
>       }
>   
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move13.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move13.C
> new file mode 100644
> index 00000000000..80e7d80cd02
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move13.C
> @@ -0,0 +1,61 @@
> +// PR c++/107363
> +// { 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); }
> +}
> +
> +template <typename T, typename U>
> +struct Optional {
> +  U &value();
> +  T release_value() {
> +    T t = std::move (value ());
> +    return t;
> +  }
> +};
> +
> +struct Foo {};
> +void test(Optional<const Foo, const Foo> o) { o.release_value(); }
> +
> +struct F {
> +  F(const F&);
> +  F(F&&) = delete;
> +};
> +
> +struct Z {
> +  Z(const Z&) = delete;
> +  Z(Z&&) = delete;
> +  Z(const Z&&);
> +};
> +
> +const F& constfref();
> +const Z& constzref();
> +
> +void
> +g ()
> +{
> +  // Will call F::F(const F&) w/ and w/o std::move.  So it's redundant.
> +  F f = std::move (constfref()); // { dg-warning "redundant move in initialization" }
> +  (void) f;
> +  // Will call Z::Z(const Z&&) w/ std::move, and Z::Z(const Z&) w/o.
> +  // So it's not redundant.
> +  Z z = std::move (constzref());
> +  (void) z;
> +}
> 
> base-commit: e583c86f49b9ef6991b25308a0ad60de9697f24a