Re: [PATCH v2] c++: Tweak for -Wpessimizing-move in templates [PR89780]
On 8/8/22 12:51, Marek Polacek wrote:
On Sat, Aug 06, 2022 at 04:02:13PM -0700, Jason Merrill wrote:
On 8/4/22 11:46, Marek Polacek wrote:
In my previous patches I've been extending our std::move warnings,
but this tweak actually dials it down a little bit. As reported in
bug 89780, it's questionable to warn about expressions in templates
that were type-dependent, but aren't anymore because we're instantiating
the template. As in,
template
Dest withMove() {
T x;
return std::move(x);
}
template Dest withMove(); // #1
template Dest withMove(); // #2
Saying that the std::move is pessimizing for #1 is not incorrect, but
it's not useful, because removing the std::move would then pessimize #2.
So the user can't really win. At the same time, disabling the warning
just because we're in a template would be going too far, I still want to
warn for
template
Dest withMove() {
Dest x;
return std::move(x);
}
because the std::move therein will be pessimizing for any instantiation.
So I'm using the suppress_warning machinery to that effect.
Problem: I had to add a new group to nowarn_spec_t, otherwise
suppressing the -Wpessimizing-move warning would disable a whole bunch
of other warnings, which we really don't want.
Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
PR c++/89780
gcc/cp/ChangeLog:
* pt.cc (tsubst_copy_and_build) : Maybe suppress
-Wpessimizing-move.
* typeck.cc (maybe_warn_pessimizing_move): Don't issue warnings
if they are suppressed.
(check_return_expr): Disable -Wpessimizing-move when returning
a dependent expression.
gcc/ChangeLog:
* diagnostic-spec.cc (nowarn_spec_t::nowarn_spec_t): Handle
OPT_Wpessimizing_move and OPT_Wredundant_move.
* diagnostic-spec.h (nowarn_spec_t): Add NW_REDUNDANT enumerator.
gcc/testsuite/ChangeLog:
* g++.dg/cpp0x/Wpessimizing-move3.C: Remove dg-warning.
* g++.dg/cpp0x/Wpessimizing-move7.C: Likewise.
* g++.dg/cpp0x/Wredundant-move2.C: Likewise.
* g++.dg/cpp0x/Wpessimizing-move9.C: New test.
---
gcc/cp/pt.cc | 3 +
gcc/cp/typeck.cc | 20 +++--
gcc/diagnostic-spec.cc| 7 +-
gcc/diagnostic-spec.h | 4 +-
.../g++.dg/cpp0x/Wpessimizing-move3.C | 2 +-
.../g++.dg/cpp0x/Wpessimizing-move7.C | 2 +-
.../g++.dg/cpp0x/Wpessimizing-move9.C | 89 +++
gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C | 4 +-
8 files changed, 119 insertions(+), 12 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C
diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
index 6c581fe0fb7..fe7e809fc2d 100644
--- a/gcc/cp/pt.cc
+++ b/gcc/cp/pt.cc
@@ -21215,6 +21215,9 @@ tsubst_copy_and_build (tree t,
CALL_EXPR_ORDERED_ARGS (call) = ord;
CALL_EXPR_REVERSE_ARGS (call) = rev;
}
+ if (warning_suppressed_p (t, OPT_Wpessimizing_move))
+ /* This also suppresses -Wredundant-move. */
+ suppress_warning (ret, OPT_Wpessimizing_move);
}
RETURN (ret);
diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
index 2650beb780e..70a5efc45de 100644
--- a/gcc/cp/typeck.cc
+++ b/gcc/cp/typeck.cc
@@ -10430,9 +10430,10 @@ maybe_warn_pessimizing_move (tree expr, tree type,
bool return_p)
if (can_do_rvo_p (arg, type))
{
auto_diagnostic_group d;
- if (warning_at (loc, OPT_Wpessimizing_move,
- "moving a temporary object prevents copy "
- "elision"))
+ if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
I don't think we ever want to suppress this warning; moving it to a
different warning flag (as I suggested on the other patch) would accomplish
that.
Agreed. I just removed the warning_suppressed_p check though, a new flag would
need another NW_ group etc.
+ && warning_at (loc, OPT_Wpessimizing_move,
+"moving a temporary object prevents copy "
+"elision"))
inform (loc, "remove % call");
}
/* The rest of the warnings is only relevant for when we are
@@ -10443,14 +10444,16 @@ maybe_warn_pessimizing_move (tree expr, tree type,
bool return_p)
else if (can_do_nrvo_p (arg, type))
{
auto_diagnostic_group d;
- if (warning_at (loc, OPT_Wpessimizing_move,
- "moving a local object in a return statement "
- "prevents copy elision"))
+ if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
+ && warning_at (loc, OPT_Wpessimizing_move,
+"moving a local object in a
[PATCH v2] c++: Tweak for -Wpessimizing-move in templates [PR89780]
On Sat, Aug 06, 2022 at 04:02:13PM -0700, Jason Merrill wrote:
> On 8/4/22 11:46, Marek Polacek wrote:
> > In my previous patches I've been extending our std::move warnings,
> > but this tweak actually dials it down a little bit. As reported in
> > bug 89780, it's questionable to warn about expressions in templates
> > that were type-dependent, but aren't anymore because we're instantiating
> > the template. As in,
> >
> >template
> >Dest withMove() {
> > T x;
> > return std::move(x);
> >}
> >
> >template Dest withMove(); // #1
> >template Dest withMove(); // #2
> >
> > Saying that the std::move is pessimizing for #1 is not incorrect, but
> > it's not useful, because removing the std::move would then pessimize #2.
> > So the user can't really win. At the same time, disabling the warning
> > just because we're in a template would be going too far, I still want to
> > warn for
> >
> >template
> >Dest withMove() {
> > Dest x;
> > return std::move(x);
> >}
> >
> > because the std::move therein will be pessimizing for any instantiation.
> >
> > So I'm using the suppress_warning machinery to that effect.
> > Problem: I had to add a new group to nowarn_spec_t, otherwise
> > suppressing the -Wpessimizing-move warning would disable a whole bunch
> > of other warnings, which we really don't want.
> >
> > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk?
> >
> > PR c++/89780
> >
> > gcc/cp/ChangeLog:
> >
> > * pt.cc (tsubst_copy_and_build) : Maybe suppress
> > -Wpessimizing-move.
> > * typeck.cc (maybe_warn_pessimizing_move): Don't issue warnings
> > if they are suppressed.
> > (check_return_expr): Disable -Wpessimizing-move when returning
> > a dependent expression.
> >
> > gcc/ChangeLog:
> >
> > * diagnostic-spec.cc (nowarn_spec_t::nowarn_spec_t): Handle
> > OPT_Wpessimizing_move and OPT_Wredundant_move.
> > * diagnostic-spec.h (nowarn_spec_t): Add NW_REDUNDANT enumerator.
> >
> > gcc/testsuite/ChangeLog:
> >
> > * g++.dg/cpp0x/Wpessimizing-move3.C: Remove dg-warning.
> > * g++.dg/cpp0x/Wpessimizing-move7.C: Likewise.
> > * g++.dg/cpp0x/Wredundant-move2.C: Likewise.
> > * g++.dg/cpp0x/Wpessimizing-move9.C: New test.
> > ---
> > gcc/cp/pt.cc | 3 +
> > gcc/cp/typeck.cc | 20 +++--
> > gcc/diagnostic-spec.cc| 7 +-
> > gcc/diagnostic-spec.h | 4 +-
> > .../g++.dg/cpp0x/Wpessimizing-move3.C | 2 +-
> > .../g++.dg/cpp0x/Wpessimizing-move7.C | 2 +-
> > .../g++.dg/cpp0x/Wpessimizing-move9.C | 89 +++
> > gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C | 4 +-
> > 8 files changed, 119 insertions(+), 12 deletions(-)
> > create mode 100644 gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C
> >
> > diff --git a/gcc/cp/pt.cc b/gcc/cp/pt.cc
> > index 6c581fe0fb7..fe7e809fc2d 100644
> > --- a/gcc/cp/pt.cc
> > +++ b/gcc/cp/pt.cc
> > @@ -21215,6 +21215,9 @@ tsubst_copy_and_build (tree t,
> > CALL_EXPR_ORDERED_ARGS (call) = ord;
> > CALL_EXPR_REVERSE_ARGS (call) = rev;
> > }
> > + if (warning_suppressed_p (t, OPT_Wpessimizing_move))
> > + /* This also suppresses -Wredundant-move. */
> > + suppress_warning (ret, OPT_Wpessimizing_move);
> > }
> > RETURN (ret);
> > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc
> > index 2650beb780e..70a5efc45de 100644
> > --- a/gcc/cp/typeck.cc
> > +++ b/gcc/cp/typeck.cc
> > @@ -10430,9 +10430,10 @@ maybe_warn_pessimizing_move (tree expr, tree type,
> > bool return_p)
> > if (can_do_rvo_p (arg, type))
> > {
> > auto_diagnostic_group d;
> > - if (warning_at (loc, OPT_Wpessimizing_move,
> > - "moving a temporary object prevents copy "
> > - "elision"))
> > + if (!warning_suppressed_p (expr, OPT_Wpessimizing_move)
>
> I don't think we ever want to suppress this warning; moving it to a
> different warning flag (as I suggested on the other patch) would accomplish
> that.
Agreed. I just removed the warning_suppressed_p check though, a new flag would
need another NW_ group etc.
> > + && warning_at (loc, OPT_Wpessimizing_move,
> > +"moving a temporary object prevents copy "
> > +"elision"))
> > inform (loc, "remove % call");
> > }
> > /* The rest of the warnings is only relevant for when we are
> > @@ -10443,14 +10444,16 @@ maybe_warn_pessimizing_move (tree expr, tree
> > type, bool return_p)
> > else if (can_do_nrvo_p (arg, type))
> > {
> > auto_diagnostic_group d;
> > - if (warning_at (loc, OPT_Wpessimizing_move,
> > - "moving a local object in a return statement "
> > -
