From: Jason Merrill <jason@redhat.com>
To: Marek Polacek <polacek@redhat.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
Date: Sat, 6 Aug 2022 16:07:54 -0700 [thread overview]
Message-ID: <242f9ff9-18d8-1da0-f9c5-51615862d22a@redhat.com> (raw)
In-Reply-To: <50df8e96-3894-b9d2-dbce-a886c04e9815@redhat.com>
On 8/6/22 15:49, Jason Merrill wrote:
> On 7/27/22 17:14, Marek Polacek wrote:
>> We already have a warning that warns about pessimizing std::move
>> in a return statement, when it prevents the NRVO:
>>
>> T fn()
>> {
>> T t;
>> return std::move (t); // warning \o/
>> }
>>
>> However, the warning doesn't warn when what we are returning is a class
>> prvalue, that is, when std::move prevents the RVO:
>>
>> T fn()
>> {
>> T t;
>> return std::move (T{}); // no warning :-(
>> }
>>
>> This came up recently in GCC:
>> <https://gcc.gnu.org/pipermail/gcc-patches/2022-July/598177.html>.
>>
>> This patch fixes that. I would like to extend the warning further, so
>> that it warns in more contexts, e.g.:
>>
>> T t = std::move(T());
>>
>> or
>>
>> void foo (T);
>> foo (std::move(T()));
>>
>> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
>>
>> PR c++/106276
>>
>> gcc/cp/ChangeLog:
>>
>> * typeck.cc (can_do_rvo_p): New.
>> (maybe_warn_pessimizing_move): Warn when moving a temporary object
>> in a return statement prevents copy elision.
>>
>> gcc/testsuite/ChangeLog:
>>
>> * g++.dg/cpp0x/Wpessimizing-move7.C: New test.
>> ---
>> gcc/cp/typeck.cc | 31 ++++++++-
>> .../g++.dg/cpp0x/Wpessimizing-move7.C | 63 +++++++++++++++++++
>> 2 files changed, 91 insertions(+), 3 deletions(-)
>> create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
>>
>> diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
>> index 6e4f23af982..9500c4e2fe8 100644
>> --- a/gcc/cp/typeck.cc
>> +++ b/gcc/cp/typeck.cc
>> @@ -10287,12 +10287,29 @@ can_do_nrvo_p (tree retval, tree functype)
>> /* The cv-unqualified type of the returned value must be the
>> same as the cv-unqualified return type of the
>> function. */
>> - && same_type_p ((TYPE_MAIN_VARIANT (TREE_TYPE (retval))),
>> - (TYPE_MAIN_VARIANT (functype)))
>> + && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
>> + TYPE_MAIN_VARIANT (functype))
>> /* And the returned value must be non-volatile. */
>> && !TYPE_VOLATILE (TREE_TYPE (retval)));
>> }
>> +/* Like can_do_nrvo_p, but we check if we're trying to move a class
>> + prvalue. */
>> +
>> +static bool
>> +can_do_rvo_p (tree retval, tree functype)
>> +{
>> + if (functype == error_mark_node)
>> + return false;
>> + if (retval)
>> + STRIP_ANY_LOCATION_WRAPPER (retval);
>> + return (retval != NULL_TREE
>> + && TREE_CODE (retval) == TARGET_EXPR
>
> Maybe use !glvalue_p instead of specifically checking for TARGET_EXPR? I
> don't feel strongly about this.
>
>> + && same_type_p (TYPE_MAIN_VARIANT (TREE_TYPE (retval)),
>> + TYPE_MAIN_VARIANT (functype))
>> + && !TYPE_VOLATILE (TREE_TYPE (retval)));
>> +}
>> +
>> /* If we should treat RETVAL, an expression being returned, as if it
>> were
>> designated by an rvalue, returns it adjusted accordingly;
>> otherwise, returns
>> NULL_TREE. See [class.copy.elision]. RETURN_P is true if this
>> is a return
>> @@ -10401,12 +10418,20 @@ maybe_warn_pessimizing_move (tree retval,
>> tree functype)
>> "prevents copy elision"))
>> inform (loc, "remove %<std::move%> call");
>> }
>> + else if (can_do_rvo_p (arg, functype))
>> + {
>> + auto_diagnostic_group d;
>> + if (warning_at (loc, OPT_Wpessimizing_move,
>> + "moving a temporary object in a return statement "
>> + "prevents copy elision"))
>
> It doesn't just prevent copy elision, it produces a dangling reference.
> This is a special case of the warning we talked about passing a
> temporary to a function that returns a reference argument unchanged, and
> should probably use a different warning flag.
Wait, no, I'm confused, the temporary does live long enough to get copied.
I still don't think we want to suppress this warning in the other patch.
>> + 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
>> && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true)))
>> {
>> - /* Make sure that the overload resolution would actually
>> succeed
>> + /* Make sure that overload resolution would actually succeed
>> if we removed the std::move call. */
>> tree t = convert_for_initialization (NULL_TREE, functype,
>> moved,
>> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
>> b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
>> new file mode 100644
>> index 00000000000..cd4eaa09aae
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C
>> @@ -0,0 +1,63 @@
>> +// 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); };
>> +
>> +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" }
>> +}
>> +
>> +B fn2 ()
>> +{
>> + return std::move (A());
>> + return std::move (A{});
>> + return std::move (foo ());
>> +}
>> +
>> +template <typename T1, typename T2>
>> +T1
>> +fn3 ()
>> +{
>> + return std::move (T2{}); // { dg-warning "moving a temporary object
>> in a return statement prevents copy elision" }
>> +}
>> +
>> +void
>> +do_fn3 ()
>> +{
>> + fn3<A, A>();
>> + fn3<B, A>();
>> +}
>> +
>> +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(char *);
>> +};
>>
>> base-commit: 219f86495791fd27b012678f13e10cada211daab
>
next prev parent reply other threads:[~2022-08-06 23:08 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-28 0:14 Marek Polacek
2022-08-06 22:49 ` Jason Merrill
2022-08-06 23:07 ` Jason Merrill [this message]
2022-08-08 19:27 ` [PATCH v2] " Marek Polacek
2022-08-11 22:27 ` Jason Merrill
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=242f9ff9-18d8-1da0-f9c5-51615862d22a@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).