* [PATCH] c++: Tweak for -Wpessimizing-move in templates [PR89780] @ 2022-08-04 18:46 Marek Polacek 2022-08-06 23:02 ` Jason Merrill 0 siblings, 1 reply; 4+ messages in thread From: Marek Polacek @ 2022-08-04 18:46 UTC (permalink / raw) To: GCC Patches, Jason Merrill 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 <typename T> Dest withMove() { T x; return std::move(x); } template Dest withMove<Dest>(); // #1 template Dest withMove<Source>(); // #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 <typename> 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) <case CALL_EXPR>: 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) + && warning_at (loc, OPT_Wpessimizing_move, + "moving a temporary object prevents copy " + "elision")) inform (loc, "remove %<std::move%> 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 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 + && !warning_suppressed_p (expr, OPT_Wredundant_move) && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) { /* Make sure that overload resolution would actually succeed @@ -10695,6 +10698,11 @@ check_return_expr (tree retval, bool *no_warning) /* We don't know if this is an lvalue or rvalue use, but either way we can mark it as read. */ mark_exp_read (retval); + /* Disable our std::move warnings when we're returning + a dependent expression (c++/89780). */ + if (retval && TREE_CODE (retval) == CALL_EXPR) + /* This also suppresses -Wredundant-move. */ + suppress_warning (retval, OPT_Wpessimizing_move); return retval; } diff --git a/gcc/diagnostic-spec.cc b/gcc/diagnostic-spec.cc index 4341ccfaae9..aece89619e7 100644 --- a/gcc/diagnostic-spec.cc +++ b/gcc/diagnostic-spec.cc @@ -96,7 +96,7 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) case OPT_Winit_self: case OPT_Wuninitialized: case OPT_Wmaybe_uninitialized: - m_bits = NW_UNINIT; + m_bits = NW_UNINIT; break; case OPT_Wdangling_pointer_: @@ -105,6 +105,11 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) m_bits = NW_DANGLING; break; + case OPT_Wpessimizing_move: + case OPT_Wredundant_move: + m_bits = NW_REDUNDANT; + break; + default: /* A catchall group for everything else. */ m_bits = NW_OTHER; diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h index 28e5e5ccc75..e5f1c127d4f 100644 --- a/gcc/diagnostic-spec.h +++ b/gcc/diagnostic-spec.h @@ -45,9 +45,11 @@ public: NW_DANGLING = 1 << 5, /* All other unclassified warnings. */ NW_OTHER = 1 << 6, + /* Warnings about redundant calls. */ + NW_REDUNDANT = 1 << 7, /* All groups of warnings. */ NW_ALL = (NW_ACCESS | NW_LEXICAL | NW_NONNULL - | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_OTHER) + | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_REDUNDANT | NW_OTHER) }; nowarn_spec_t (): m_bits () { } diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C index a1af1230b68..c81f29a4ba6 100644 --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C @@ -39,7 +39,7 @@ Tp1 fn2 () { Tp2 t; - return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } + return std::move (t); } template<typename Tp1, typename Tp2> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C index a17c7a87b7d..ebbcb2122a6 100644 --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C @@ -46,7 +46,7 @@ template <typename T1, typename T2> T1 fn3 () { - return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" } + return std::move (T2{}); } void diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C new file mode 100644 index 00000000000..9519695eda7 --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C @@ -0,0 +1,89 @@ +// PR c++/89780 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpessimizing-move -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 Dest { + Dest() = default; + Dest(Dest&&); + Dest(const Dest&); +}; +struct Source : Dest {}; + +template <typename T> +Dest withMove() { + T x; + return std::move(x); +} + +template Dest withMove<Dest>(); +template Dest withMove<Source>(); + +template<typename T> +Dest bar () { + return std::move(T()); +} + +template Dest bar<Dest>(); +template Dest bar<Source>(); + +template<typename T> +Dest baz (T x) { + return std::move(x); +} + +void +call_baz () +{ + Dest d; + Source s; + baz (d); + baz (s); +} + +template<typename> +Dest foo () +{ + Dest d; + return std::move(d); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template Dest foo<int>(); + +template<typename> +Dest qux () { + return std::move(Dest()); // { dg-warning "moving a temporary object prevents copy elision" } +} + +template Dest qux<int>(); + +template<typename> +Dest qui (Dest x) { + return std::move(x); // { dg-warning "redundant move in return statement" } +} + +void +call_qui () +{ + Dest d; + qui<int> (d); +} diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C index f181afeeb84..6e0aa4b0277 100644 --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C @@ -37,14 +37,14 @@ template<typename Tp1, typename Tp2> Tp1 fn2 (Tp2 t) { - return std::move (t); // { dg-warning "redundant move in return statement" } + return std::move (t); } template<typename Tp1, typename Tp2> Tp1 fn3 (Tp2 t) { - return std::move (t); // { dg-warning "redundant move in return statement" } + return std::move (t); } int base-commit: be58bf98e98bb431ed26ca8be84586075fe8be82 prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5 prerequisite-patch-id: ce490c3c0b991fa7f27315387949c145c66223a4 -- 2.37.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] c++: Tweak for -Wpessimizing-move in templates [PR89780] 2022-08-04 18:46 [PATCH] c++: Tweak for -Wpessimizing-move in templates [PR89780] Marek Polacek @ 2022-08-06 23:02 ` Jason Merrill 2022-08-08 19:51 ` [PATCH v2] " Marek Polacek 0 siblings, 1 reply; 4+ messages in thread From: Jason Merrill @ 2022-08-06 23:02 UTC (permalink / raw) To: Marek Polacek, GCC Patches 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 <typename T> > Dest withMove() { > T x; > return std::move(x); > } > > template Dest withMove<Dest>(); // #1 > template Dest withMove<Source>(); // #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 <typename> > 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) <case CALL_EXPR>: 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. > + && warning_at (loc, OPT_Wpessimizing_move, > + "moving a temporary object prevents copy " > + "elision")) > inform (loc, "remove %<std::move%> 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 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 > + && !warning_suppressed_p (expr, OPT_Wredundant_move) > && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) > { > /* Make sure that overload resolution would actually succeed > @@ -10695,6 +10698,11 @@ check_return_expr (tree retval, bool *no_warning) > /* We don't know if this is an lvalue or rvalue use, but > either way we can mark it as read. */ > mark_exp_read (retval); > + /* Disable our std::move warnings when we're returning > + a dependent expression (c++/89780). */ > + if (retval && TREE_CODE (retval) == CALL_EXPR) Should this check type_dependent_expression_p? > + /* This also suppresses -Wredundant-move. */ > + suppress_warning (retval, OPT_Wpessimizing_move); > return retval; > } > > diff --git a/gcc/diagnostic-spec.cc b/gcc/diagnostic-spec.cc > index 4341ccfaae9..aece89619e7 100644 > --- a/gcc/diagnostic-spec.cc > +++ b/gcc/diagnostic-spec.cc > @@ -96,7 +96,7 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) > case OPT_Winit_self: > case OPT_Wuninitialized: > case OPT_Wmaybe_uninitialized: > - m_bits = NW_UNINIT; > + m_bits = NW_UNINIT; > break; > > case OPT_Wdangling_pointer_: > @@ -105,6 +105,11 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) > m_bits = NW_DANGLING; > break; > > + case OPT_Wpessimizing_move: > + case OPT_Wredundant_move: > + m_bits = NW_REDUNDANT; > + break; > + > default: > /* A catchall group for everything else. */ > m_bits = NW_OTHER; > diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h > index 28e5e5ccc75..e5f1c127d4f 100644 > --- a/gcc/diagnostic-spec.h > +++ b/gcc/diagnostic-spec.h > @@ -45,9 +45,11 @@ public: > NW_DANGLING = 1 << 5, > /* All other unclassified warnings. */ > NW_OTHER = 1 << 6, > + /* Warnings about redundant calls. */ > + NW_REDUNDANT = 1 << 7, > /* All groups of warnings. */ > NW_ALL = (NW_ACCESS | NW_LEXICAL | NW_NONNULL > - | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_OTHER) > + | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_REDUNDANT | NW_OTHER) > }; > > nowarn_spec_t (): m_bits () { } > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > index a1af1230b68..c81f29a4ba6 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > @@ -39,7 +39,7 @@ Tp1 > fn2 () > { > Tp2 t; > - return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } > + return std::move (t); > } > > template<typename Tp1, typename Tp2> > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > index a17c7a87b7d..ebbcb2122a6 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > @@ -46,7 +46,7 @@ template <typename T1, typename T2> > T1 > fn3 () > { > - return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" } > + return std::move (T2{}); As mentioned above, we want to keep this warning; the dangling reference problem is not type-dependent. > } > > void > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C > new file mode 100644 > index 00000000000..9519695eda7 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C > @@ -0,0 +1,89 @@ > +// PR c++/89780 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wpessimizing-move -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 Dest { > + Dest() = default; > + Dest(Dest&&); > + Dest(const Dest&); > +}; > +struct Source : Dest {}; > + > +template <typename T> > +Dest withMove() { > + T x; > + return std::move(x); > +} > + > +template Dest withMove<Dest>(); > +template Dest withMove<Source>(); > + > +template<typename T> > +Dest bar () { > + return std::move(T()); > +} > + > +template Dest bar<Dest>(); > +template Dest bar<Source>(); > + > +template<typename T> > +Dest baz (T x) { > + return std::move(x); > +} > + > +void > +call_baz () > +{ > + Dest d; > + Source s; > + baz (d); > + baz (s); > +} > + > +template<typename> > +Dest foo () > +{ > + Dest d; > + return std::move(d); // { dg-warning "moving a local object in a return statement prevents copy elision" } > +} > + > +template Dest foo<int>(); > + > +template<typename> > +Dest qux () { > + return std::move(Dest()); // { dg-warning "moving a temporary object prevents copy elision" } > +} > + > +template Dest qux<int>(); > + > +template<typename> > +Dest qui (Dest x) { > + return std::move(x); // { dg-warning "redundant move in return statement" } > +} > + > +void > +call_qui () > +{ > + Dest d; > + qui<int> (d); > +} > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > index f181afeeb84..6e0aa4b0277 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > @@ -37,14 +37,14 @@ template<typename Tp1, typename Tp2> > Tp1 > fn2 (Tp2 t) > { > - return std::move (t); // { dg-warning "redundant move in return statement" } > + return std::move (t); > } > > template<typename Tp1, typename Tp2> > Tp1 > fn3 (Tp2 t) > { > - return std::move (t); // { dg-warning "redundant move in return statement" } > + return std::move (t); > } > > int > > base-commit: be58bf98e98bb431ed26ca8be84586075fe8be82 > prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5 > prerequisite-patch-id: ce490c3c0b991fa7f27315387949c145c66223a4 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2] c++: Tweak for -Wpessimizing-move in templates [PR89780] 2022-08-06 23:02 ` Jason Merrill @ 2022-08-08 19:51 ` Marek Polacek 2022-08-11 22:24 ` Jason Merrill 0 siblings, 1 reply; 4+ messages in thread From: Marek Polacek @ 2022-08-08 19:51 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches 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 <typename T> > > Dest withMove() { > > T x; > > return std::move(x); > > } > > > > template Dest withMove<Dest>(); // #1 > > template Dest withMove<Source>(); // #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 <typename> > > 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) <case CALL_EXPR>: 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 %<std::move%> 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 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 > > + && !warning_suppressed_p (expr, OPT_Wredundant_move) > > && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) > > { > > /* Make sure that overload resolution would actually succeed > > @@ -10695,6 +10698,11 @@ check_return_expr (tree retval, bool *no_warning) > > /* We don't know if this is an lvalue or rvalue use, but > > either way we can mark it as read. */ > > mark_exp_read (retval); > > + /* Disable our std::move warnings when we're returning > > + a dependent expression (c++/89780). */ > > + if (retval && TREE_CODE (retval) == CALL_EXPR) > > Should this check type_dependent_expression_p? We already check that, this block is guarded by: if (dependent_type_p (functype) || type_dependent_expression_p (retval)) { dependent: > > --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C > > @@ -46,7 +46,7 @@ template <typename T1, typename T2> > > T1 > > fn3 () > > { > > - return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" } > > + return std::move (T2{}); > > As mentioned above, we want to keep this warning; the dangling reference > problem is not type-dependent. Fixed. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- 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 <typename T> Dest withMove() { T x; return std::move(x); } template Dest withMove<Dest>(); // #1 template Dest withMove<Source>(); // #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 <typename> 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. PR c++/89780 gcc/cp/ChangeLog: * pt.cc (tsubst_copy_and_build) <case CALL_EXPR>: 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/Wredundant-move2.C: Likewise. --- gcc/cp/pt.cc | 3 + gcc/cp/typeck.cc | 13 ++- gcc/diagnostic-spec.cc | 7 +- gcc/diagnostic-spec.h | 4 +- .../g++.dg/cpp0x/Wpessimizing-move3.C | 2 +- .../g++.dg/cpp0x/Wpessimizing-move9.C | 89 +++++++++++++++++++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C | 4 +- 7 files changed, 114 insertions(+), 8 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..f4104c75fbe 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -10443,14 +10443,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 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 + && !warning_suppressed_p (expr, OPT_Wredundant_move) && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) { /* Make sure that overload resolution would actually succeed @@ -10695,6 +10697,11 @@ check_return_expr (tree retval, bool *no_warning) /* We don't know if this is an lvalue or rvalue use, but either way we can mark it as read. */ mark_exp_read (retval); + /* Disable our std::move warnings when we're returning + a dependent expression (c++/89780). */ + if (retval && TREE_CODE (retval) == CALL_EXPR) + /* This also suppresses -Wredundant-move. */ + suppress_warning (retval, OPT_Wpessimizing_move); return retval; } diff --git a/gcc/diagnostic-spec.cc b/gcc/diagnostic-spec.cc index 4341ccfaae9..aece89619e7 100644 --- a/gcc/diagnostic-spec.cc +++ b/gcc/diagnostic-spec.cc @@ -96,7 +96,7 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) case OPT_Winit_self: case OPT_Wuninitialized: case OPT_Wmaybe_uninitialized: - m_bits = NW_UNINIT; + m_bits = NW_UNINIT; break; case OPT_Wdangling_pointer_: @@ -105,6 +105,11 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) m_bits = NW_DANGLING; break; + case OPT_Wpessimizing_move: + case OPT_Wredundant_move: + m_bits = NW_REDUNDANT; + break; + default: /* A catchall group for everything else. */ m_bits = NW_OTHER; diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h index 28e5e5ccc75..e5f1c127d4f 100644 --- a/gcc/diagnostic-spec.h +++ b/gcc/diagnostic-spec.h @@ -45,9 +45,11 @@ public: NW_DANGLING = 1 << 5, /* All other unclassified warnings. */ NW_OTHER = 1 << 6, + /* Warnings about redundant calls. */ + NW_REDUNDANT = 1 << 7, /* All groups of warnings. */ NW_ALL = (NW_ACCESS | NW_LEXICAL | NW_NONNULL - | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_OTHER) + | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_REDUNDANT | NW_OTHER) }; nowarn_spec_t (): m_bits () { } diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C index a1af1230b68..c81f29a4ba6 100644 --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C @@ -39,7 +39,7 @@ Tp1 fn2 () { Tp2 t; - return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } + return std::move (t); } template<typename Tp1, typename Tp2> diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C new file mode 100644 index 00000000000..898040e6bfc --- /dev/null +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C @@ -0,0 +1,89 @@ +// PR c++/89780 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpessimizing-move -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 Dest { + Dest() = default; + Dest(Dest&&); + Dest(const Dest&); +}; +struct Source : Dest {}; + +template <typename T> +Dest withMove() { + T x; + return std::move(x); +} + +template Dest withMove<Dest>(); +template Dest withMove<Source>(); + +template<typename T> +Dest bar () { + return std::move(T()); // { dg-warning "moving a temporary object prevents copy elision" } +} + +template Dest bar<Dest>(); +template Dest bar<Source>(); + +template<typename T> +Dest baz (T x) { + return std::move(x); +} + +void +call_baz () +{ + Dest d; + Source s; + baz (d); + baz (s); +} + +template<typename> +Dest foo () +{ + Dest d; + return std::move(d); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template Dest foo<int>(); + +template<typename> +Dest qux () { + return std::move(Dest()); // { dg-warning "moving a temporary object prevents copy elision" } +} + +template Dest qux<int>(); + +template<typename> +Dest qui (Dest x) { + return std::move(x); // { dg-warning "redundant move in return statement" } +} + +void +call_qui () +{ + Dest d; + qui<int> (d); +} diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C index f181afeeb84..6e0aa4b0277 100644 --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C @@ -37,14 +37,14 @@ template<typename Tp1, typename Tp2> Tp1 fn2 (Tp2 t) { - return std::move (t); // { dg-warning "redundant move in return statement" } + return std::move (t); } template<typename Tp1, typename Tp2> Tp1 fn3 (Tp2 t) { - return std::move (t); // { dg-warning "redundant move in return statement" } + return std::move (t); } int base-commit: 4b0253b019943abf2cc5f4db0b7ed67caedffe4a prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5 prerequisite-patch-id: ce490c3c0b991fa7f27315387949c145c66223a4 -- 2.37.1 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2] c++: Tweak for -Wpessimizing-move in templates [PR89780] 2022-08-08 19:51 ` [PATCH v2] " Marek Polacek @ 2022-08-11 22:24 ` Jason Merrill 0 siblings, 0 replies; 4+ messages in thread From: Jason Merrill @ 2022-08-11 22:24 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches 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 <typename T> >>> Dest withMove() { >>> T x; >>> return std::move(x); >>> } >>> >>> template Dest withMove<Dest>(); // #1 >>> template Dest withMove<Source>(); // #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 <typename> >>> 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) <case CALL_EXPR>: 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 %<std::move%> 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 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 >>> + && !warning_suppressed_p (expr, OPT_Wredundant_move) >>> && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) >>> { >>> /* Make sure that overload resolution would actually succeed >>> @@ -10695,6 +10698,11 @@ check_return_expr (tree retval, bool *no_warning) >>> /* We don't know if this is an lvalue or rvalue use, but >>> either way we can mark it as read. */ >>> mark_exp_read (retval); >>> + /* Disable our std::move warnings when we're returning >>> + a dependent expression (c++/89780). */ >>> + if (retval && TREE_CODE (retval) == CALL_EXPR) >> >> Should this check type_dependent_expression_p? > > We already check that, this block is guarded by: > > if (dependent_type_p (functype) > || type_dependent_expression_p (retval)) > { > dependent: > >>> --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C >>> +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move7.C >>> @@ -46,7 +46,7 @@ template <typename T1, typename T2> >>> T1 >>> fn3 () >>> { >>> - return std::move (T2{}); // { dg-warning "moving a temporary object prevents copy elision" } >>> + return std::move (T2{}); >> >> As mentioned above, we want to keep this warning; the dangling reference >> problem is not type-dependent. > > Fixed. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK. > -- >8 -- > 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 <typename T> > Dest withMove() { > T x; > return std::move(x); > } > > template Dest withMove<Dest>(); // #1 > template Dest withMove<Source>(); // #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 <typename> > 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. > > PR c++/89780 > > gcc/cp/ChangeLog: > > * pt.cc (tsubst_copy_and_build) <case CALL_EXPR>: 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/Wredundant-move2.C: Likewise. > --- > gcc/cp/pt.cc | 3 + > gcc/cp/typeck.cc | 13 ++- > gcc/diagnostic-spec.cc | 7 +- > gcc/diagnostic-spec.h | 4 +- > .../g++.dg/cpp0x/Wpessimizing-move3.C | 2 +- > .../g++.dg/cpp0x/Wpessimizing-move9.C | 89 +++++++++++++++++++ > gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C | 4 +- > 7 files changed, 114 insertions(+), 8 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..f4104c75fbe 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -10443,14 +10443,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 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 > + && !warning_suppressed_p (expr, OPT_Wredundant_move) > && (moved = treat_lvalue_as_rvalue_p (arg, /*return*/true))) > { > /* Make sure that overload resolution would actually succeed > @@ -10695,6 +10697,11 @@ check_return_expr (tree retval, bool *no_warning) > /* We don't know if this is an lvalue or rvalue use, but > either way we can mark it as read. */ > mark_exp_read (retval); > + /* Disable our std::move warnings when we're returning > + a dependent expression (c++/89780). */ > + if (retval && TREE_CODE (retval) == CALL_EXPR) > + /* This also suppresses -Wredundant-move. */ > + suppress_warning (retval, OPT_Wpessimizing_move); > return retval; > } > > diff --git a/gcc/diagnostic-spec.cc b/gcc/diagnostic-spec.cc > index 4341ccfaae9..aece89619e7 100644 > --- a/gcc/diagnostic-spec.cc > +++ b/gcc/diagnostic-spec.cc > @@ -96,7 +96,7 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) > case OPT_Winit_self: > case OPT_Wuninitialized: > case OPT_Wmaybe_uninitialized: > - m_bits = NW_UNINIT; > + m_bits = NW_UNINIT; > break; > > case OPT_Wdangling_pointer_: > @@ -105,6 +105,11 @@ nowarn_spec_t::nowarn_spec_t (opt_code opt) > m_bits = NW_DANGLING; > break; > > + case OPT_Wpessimizing_move: > + case OPT_Wredundant_move: > + m_bits = NW_REDUNDANT; > + break; > + > default: > /* A catchall group for everything else. */ > m_bits = NW_OTHER; > diff --git a/gcc/diagnostic-spec.h b/gcc/diagnostic-spec.h > index 28e5e5ccc75..e5f1c127d4f 100644 > --- a/gcc/diagnostic-spec.h > +++ b/gcc/diagnostic-spec.h > @@ -45,9 +45,11 @@ public: > NW_DANGLING = 1 << 5, > /* All other unclassified warnings. */ > NW_OTHER = 1 << 6, > + /* Warnings about redundant calls. */ > + NW_REDUNDANT = 1 << 7, > /* All groups of warnings. */ > NW_ALL = (NW_ACCESS | NW_LEXICAL | NW_NONNULL > - | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_OTHER) > + | NW_UNINIT | NW_VFLOW | NW_DANGLING | NW_REDUNDANT | NW_OTHER) > }; > > nowarn_spec_t (): m_bits () { } > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > index a1af1230b68..c81f29a4ba6 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > @@ -39,7 +39,7 @@ Tp1 > fn2 () > { > Tp2 t; > - return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } > + return std::move (t); > } > > template<typename Tp1, typename Tp2> > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C > new file mode 100644 > index 00000000000..898040e6bfc > --- /dev/null > +++ b/gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move9.C > @@ -0,0 +1,89 @@ > +// PR c++/89780 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wpessimizing-move -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 Dest { > + Dest() = default; > + Dest(Dest&&); > + Dest(const Dest&); > +}; > +struct Source : Dest {}; > + > +template <typename T> > +Dest withMove() { > + T x; > + return std::move(x); > +} > + > +template Dest withMove<Dest>(); > +template Dest withMove<Source>(); > + > +template<typename T> > +Dest bar () { > + return std::move(T()); // { dg-warning "moving a temporary object prevents copy elision" } > +} > + > +template Dest bar<Dest>(); > +template Dest bar<Source>(); > + > +template<typename T> > +Dest baz (T x) { > + return std::move(x); > +} > + > +void > +call_baz () > +{ > + Dest d; > + Source s; > + baz (d); > + baz (s); > +} > + > +template<typename> > +Dest foo () > +{ > + Dest d; > + return std::move(d); // { dg-warning "moving a local object in a return statement prevents copy elision" } > +} > + > +template Dest foo<int>(); > + > +template<typename> > +Dest qux () { > + return std::move(Dest()); // { dg-warning "moving a temporary object prevents copy elision" } > +} > + > +template Dest qux<int>(); > + > +template<typename> > +Dest qui (Dest x) { > + return std::move(x); // { dg-warning "redundant move in return statement" } > +} > + > +void > +call_qui () > +{ > + Dest d; > + qui<int> (d); > +} > diff --git a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > index f181afeeb84..6e0aa4b0277 100644 > --- a/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > +++ b/gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C > @@ -37,14 +37,14 @@ template<typename Tp1, typename Tp2> > Tp1 > fn2 (Tp2 t) > { > - return std::move (t); // { dg-warning "redundant move in return statement" } > + return std::move (t); > } > > template<typename Tp1, typename Tp2> > Tp1 > fn3 (Tp2 t) > { > - return std::move (t); // { dg-warning "redundant move in return statement" } > + return std::move (t); > } > > int > > base-commit: 4b0253b019943abf2cc5f4db0b7ed67caedffe4a > prerequisite-patch-id: f4862bc588ce5fed1d21016fecc4b61feb71eba5 > prerequisite-patch-id: ce490c3c0b991fa7f27315387949c145c66223a4 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-11 22:25 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-04 18:46 [PATCH] c++: Tweak for -Wpessimizing-move in templates [PR89780] Marek Polacek 2022-08-06 23:02 ` Jason Merrill 2022-08-08 19:51 ` [PATCH v2] " Marek Polacek 2022-08-11 22:24 ` 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).