Re: [PATCH v2] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
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:
.
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 % 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:
.
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.
[PATCH v2] c++: Extend -Wpessimizing-move for class prvalues [PR106276]
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:
> > > .
> > >
> > > 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 % 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:
