* [PATCH] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
@ 2022-07-28 0:14 Marek Polacek
2022-08-06 22:49 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Marek Polacek @ 2022-07-28 0:14 UTC (permalink / raw)
To: GCC Patches, Jason Merrill
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
+ && 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"))
+ 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
--
2.37.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
2022-07-28 0:14 [PATCH] c++: Extend -Wpessimizing-move for class prvalues [PR106276] Marek Polacek
@ 2022-08-06 22:49 ` Jason Merrill
2022-08-06 23:07 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Jason Merrill @ 2022-08-06 22:49 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
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.
> + 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
2022-08-06 22:49 ` Jason Merrill
@ 2022-08-06 23:07 ` Jason Merrill
2022-08-08 19:27 ` [PATCH v2] " Marek Polacek
0 siblings, 1 reply; 5+ messages in thread
From: Jason Merrill @ 2022-08-06 23:07 UTC (permalink / raw)
To: Marek Polacek, GCC Patches
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
2022-08-06 23:07 ` Jason Merrill
@ 2022-08-08 19:27 ` Marek Polacek
2022-08-11 22:27 ` Jason Merrill
0 siblings, 1 reply; 5+ messages in thread
From: Marek Polacek @ 2022-08-08 19:27 UTC (permalink / raw)
To: Jason Merrill; +Cc: GCC Patches
On Sat, Aug 06, 2022 at 04:07:54PM -0700, Jason Merrill wrote:
> 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.
OK, this version uses !glvalue_p.
> > > + && 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.
Yeah, that makes sense. I'll change it there.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
-- >8 --
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()));
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..15548e5091f 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
+ && !glvalue_p (retval)
+ && 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"))
+ 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: 4b0253b019943abf2cc5f4db0b7ed67caedffe4a
--
2.37.1
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
2022-08-08 19:27 ` [PATCH v2] " Marek Polacek
@ 2022-08-11 22:27 ` Jason Merrill
0 siblings, 0 replies; 5+ messages in thread
From: Jason Merrill @ 2022-08-11 22:27 UTC (permalink / raw)
To: Marek Polacek; +Cc: GCC Patches
On 8/8/22 12:27, Marek Polacek wrote:
> On Sat, Aug 06, 2022 at 04:07:54PM -0700, Jason Merrill wrote:
>> 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.
>
> OK, this version uses !glvalue_p.
>
>>>> + && 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.
>
> Yeah, that makes sense. I'll change it there.
>
> Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
OK.
> -- >8 --
> 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()));
>
> 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..15548e5091f 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
> + && !glvalue_p (retval)
> + && 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"))
> + 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: 4b0253b019943abf2cc5f4db0b7ed67caedffe4a
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-08-11 22:28 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-28 0:14 [PATCH] c++: Extend -Wpessimizing-move for class prvalues [PR106276] Marek Polacek
2022-08-06 22:49 ` Jason Merrill
2022-08-06 23:07 ` Jason Merrill
2022-08-08 19:27 ` [PATCH v2] " Marek Polacek
2022-08-11 22:27 ` Jason Merrill
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).