public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>,
	GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] c++: Quash bogus -Wredundant-move warning
Date: Tue, 23 Aug 2022 17:53:16 -0400	[thread overview]
Message-ID: <9dee175c-dcae-80f7-2976-949740c7c616@redhat.com> (raw)
In-Reply-To: <20220823001026.109061-1-polacek@redhat.com>

On 8/22/22 17:10, Marek Polacek wrote:
> This patch fixes a pretty stoopid thinko.  When I added code to warn
> about pessimizing std::move in initializations like
> 
>    T t{std::move(T())};
> 
> I also added code to unwrap the expression from { }.  But when we have
> 
>    return {std::move(t)};
> 
> we cannot warn about a redundant std::move because the implicit move
> wouldn't happen for "return {t};" because the expression isn't just
> a name.  However, we still want to warn about
> 
>    return {std::move(T())};
> 
> so let's not disable the -Wpessimizing-move warning.  Tests added for
> both cases.
> 
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?

OK.

> gcc/cp/ChangeLog:
> 
> 	* typeck.cc (maybe_warn_pessimizing_move): Don't warn about
> 	redundant std::move when the expression was wrapped in { }.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/cpp0x/Wpessimizing-move10.C: New test.
> 	* g++.dg/cpp0x/Wredundant-move12.C: New test.
> ---
>   gcc/cp/typeck.cc                              | 13 +++++--
>   .../g++.dg/cpp0x/Wpessimizing-move10.C        | 30 ++++++++++++++++
>   .../g++.dg/cpp0x/Wredundant-move12.C          | 36 +++++++++++++++++++
>   3 files changed, 77 insertions(+), 2 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move10.C
>   create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wredundant-move12.C
> 
> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> index 992ebfd99fb..7fde65adaa4 100644
> --- a/gcc/cp/typeck.cc
> +++ b/gcc/cp/typeck.cc
> @@ -10397,11 +10397,15 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>     if (!CLASS_TYPE_P (type))
>       return;
>   
> +  bool wrapped_p = false;
>     /* A a = std::move (A());  */
>     if (TREE_CODE (expr) == TREE_LIST)
>       {
>         if (list_length (expr) == 1)
> -	expr = TREE_VALUE (expr);
> +	{
> +	  expr = TREE_VALUE (expr);
> +	  wrapped_p = true;
> +	}
>         else
>   	return;
>       }
> @@ -10410,7 +10414,10 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>     else if (TREE_CODE (expr) == CONSTRUCTOR)
>       {
>         if (CONSTRUCTOR_NELTS (expr) == 1)
> -	expr = CONSTRUCTOR_ELT (expr, 0)->value;
> +	{
> +	  expr = CONSTRUCTOR_ELT (expr, 0)->value;
> +	  wrapped_p = true;
> +	}
>         else
>   	return;
>       }
> @@ -10458,6 +10465,8 @@ maybe_warn_pessimizing_move (tree expr, tree type, bool return_p)
>         /* 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
> +	       /* This doesn't apply for return {std::move (t)};.  */
> +	       && !wrapped_p
>   	       && !warning_suppressed_p (expr, OPT_Wredundant_move)
>   	       && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
>   	{
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move10.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move10.C
> new file mode 100644
> index 00000000000..77314141da3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move10.C
> @@ -0,0 +1,30 @@
> +// { 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 S { };
> +
> +S
> +f ()
> +{
> +   return {std::move(S())}; // { dg-warning "moving a temporary object prevents copy elision" }
> +}
> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move12.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move12.C
> new file mode 100644
> index 00000000000..192d981ba5e
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move12.C
> @@ -0,0 +1,36 @@
> +// { 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(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) }; // { dg-bogus "redundant move" }
> +}
> 
> base-commit: cc4fa7a210b638d6a46f14dab17f2361389d18e1


      reply	other threads:[~2022-08-23 21:53 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-23  0:10 Marek Polacek
2022-08-23 21:53 ` Jason Merrill [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=9dee175c-dcae-80f7-2976-949740c7c616@redhat.com \
    --to=jason@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=polacek@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).