| Message ID | 20180820210849.GD4317@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | C++ PATCH for c++/86981, implement -Wpessimizing-move | expand |
On Mon, 2018-08-20 at 17:08 -0400, Marek Polacek wrote: > This patch implements -Wpessimizing-move, a C++-specific warning that > warns > when using std::move in a return statement precludes the > NRVO. Consider: > > struct T { }; > > T f() > { > T t; > return std::move(t); > } > > where we could elide the copy were it not for the move call; the > standard > requires that the expression be the name of a non-volatile automatic > object, so > no function call would work there. > Had 't' been a parameter, the move would have been merely redundant, > but that's > for another warning, -Wredundant-move, which should be a fairly easy > extension > of this one. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? [...snip...] > + /* Warn if we could do copy elision were it not for the > move. */ > + if (can_do_nrvo_p (arg, functype) > + && warning (OPT_Wpessimizing_move, "moving a local object " > + "in a return statement prevents copy elision")) > + inform (input_location, "remove %<std::move%> call"); > + } > + } As of r263675 it's now possible to tell the diagnostics subsystem that the warning and note are related by using an auto_diagnostic_group instance [1], so please can this read: if (can_do_nrvo_p (arg, functype)) { auto_diagnostic_group d; if (warning (OPT_Wpessimizing_move, "moving a local object " "in a return statement prevents copy elision")) inform (input_location, "remove %<std::move%> call"); } Dave [1] see https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01190.html Not that this does much yet, but I'm hoping to make it do so, especially for cases like this where both diagnostics share the same location
On Mon, Aug 20, 2018 at 05:18:49PM -0400, David Malcolm wrote: > As of r263675 it's now possible to tell the diagnostics subsystem that > the warning and note are related by using an auto_diagnostic_group > instance [1], so please can this read: > > if (can_do_nrvo_p (arg, functype)) > { > auto_diagnostic_group d; > if (warning (OPT_Wpessimizing_move, "moving a local object " > "in a return statement prevents copy elision")) > inform (input_location, "remove %<std::move%> call"); > } Sure: Bootstrapped/regtested on x86_64-linux, ok for trunk? 2018-08-20 Marek Polacek <polacek@redhat.com> PR c++/86981, Implement -Wpessimizing-move. * c.opt (Wpessimizing-move): New option. * typeck.c (decl_in_std_namespace_p): New. (is_std_move_p): New. (can_do_nrvo_p): New, factored out of ... (check_return_expr): ... here. Warn about potentially harmful std::move in a return statement. * doc/invoke.texi: Document -Wpessimizing-move. * g++.dg/cpp0x/Wpessimizing-move1.C: New test. * g++.dg/cpp0x/Wpessimizing-move2.C: New test. * g++.dg/cpp0x/Wpessimizing-move3.C: New test. * g++.dg/cpp0x/Wpessimizing-move4.C: New test. * g++.dg/cpp1z/Wpessimizing-move1.C: New test. diff --git gcc/c-family/c.opt gcc/c-family/c.opt index 9980bfac11c..76840dd77ad 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -937,6 +937,10 @@ Wpedantic C ObjC C++ ObjC++ CPP(cpp_pedantic) CppReason(CPP_W_PEDANTIC) Warning ; Documented in common.opt +Wpessimizing-move +C++ ObjC++ Var(warn_pessimizing_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn about calling std::move on a local object in a return statement preventing copy elision. + Wpmf-conversions C++ ObjC++ Var(warn_pmf2ptr) Init(1) Warning Warn when converting the type of pointers to member functions. diff --git gcc/cp/typeck.c gcc/cp/typeck.c index 8c13ae9b19b..5fe47299772 100644 --- gcc/cp/typeck.c +++ gcc/cp/typeck.c @@ -9113,6 +9113,58 @@ maybe_warn_about_returning_address_of_local (tree retval) return false; } +/* Returns true if DECL is in the std namespace. */ + +static bool +decl_in_std_namespace_p (tree decl) +{ + return (decl != NULL_TREE + && DECL_NAMESPACE_STD_P (decl_namespace_context (decl))); +} + +/* Returns true if FN, a CALL_EXPR, is a call to std::move. */ + +static bool +is_std_move_p (tree fn) +{ + /* std::move only takes one argument. */ + if (call_expr_nargs (fn) != 1) + return false; + + tree fndecl = cp_get_callee_fndecl_nofold (fn); + if (!decl_in_std_namespace_p (fndecl)) + return false; + + tree name = DECL_NAME (fndecl); + return name && id_equal (name, "move"); +} + +/* Returns true if RETVAL is a good candidate for the NRVO as per + [class.copy.elision]. FUNCTYPE is the type the function is declared + to return. */ + +static bool +can_do_nrvo_p (tree retval, tree functype) +{ + tree result = DECL_RESULT (current_function_decl); + return (retval != NULL_TREE + && !processing_template_decl + /* Must be a local, automatic variable. */ + && VAR_P (retval) + && DECL_CONTEXT (retval) == current_function_decl + && !TREE_STATIC (retval) + /* And not a lambda or anonymous union proxy. */ + && !DECL_HAS_VALUE_EXPR_P (retval) + && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) + /* 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))) + /* And the returned value must be non-volatile. */ + && !TYPE_VOLATILE (TREE_TYPE (retval))); +} + /* Check that returning RETVAL from the current function is valid. Return an expression explicitly showing all conversions required to change RETVAL into the function return type, and to assign it to @@ -9130,7 +9182,6 @@ check_return_expr (tree retval, bool *no_warning) the declared type is incomplete. */ tree functype; int fn_returns_value_p; - bool named_return_value_okay_p; *no_warning = false; @@ -9342,24 +9393,7 @@ check_return_expr (tree retval, bool *no_warning) See finish_function and finalize_nrv for the rest of this optimization. */ - named_return_value_okay_p = - (retval != NULL_TREE - && !processing_template_decl - /* Must be a local, automatic variable. */ - && VAR_P (retval) - && DECL_CONTEXT (retval) == current_function_decl - && ! TREE_STATIC (retval) - /* And not a lambda or anonymous union proxy. */ - && !DECL_HAS_VALUE_EXPR_P (retval) - && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) - /* 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))) - /* And the returned value must be non-volatile. */ - && ! TYPE_VOLATILE (TREE_TYPE (retval))); - + bool named_return_value_okay_p = can_do_nrvo_p (retval, functype); if (fn_returns_value_p && flag_elide_constructors) { if (named_return_value_okay_p @@ -9375,6 +9409,35 @@ check_return_expr (tree retval, bool *no_warning) if (!retval) return NULL_TREE; + /* Warn about wrong usage of std::move in a return statement. */ + if (!named_return_value_okay_p + && warn_pessimizing_move + /* C++98 doesn't know move. */ + && (cxx_dialect >= cxx11) + /* This is only interesting for class type. */ + && CLASS_TYPE_P (functype) + /* We're looking for *std::move<T&> (&arg). */ + && REFERENCE_REF_P (retval) + && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR) + { + tree fn = TREE_OPERAND (retval, 0); + if (is_std_move_p (fn)) + { + tree arg = CALL_EXPR_ARG (fn, 0); + STRIP_NOPS (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + /* Warn if we could do copy elision were it not for the move. */ + if (can_do_nrvo_p (arg, functype)) + { + auto_diagnostic_group d; + if (warning (OPT_Wpessimizing_move, "moving a local object " + "in a return statement prevents copy elision")) + inform (input_location, "remove %<std::move%> call"); + } + } + } + /* Do any required conversions. */ if (retval == result || DECL_CONSTRUCTOR_P (current_function_decl)) /* No conversions are required. */ diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index f8287153be1..7a79ba32fc7 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -231,6 +231,7 @@ in the following sections. -Wdelete-non-virtual-dtor -Wdeprecated-copy -Wliteral-suffix @gol -Wmultiple-inheritance @gol -Wnamespaces -Wnarrowing @gol +-Wpessimizing-move @gol -Wnoexcept -Wnoexcept-type -Wclass-memaccess @gol -Wnon-virtual-dtor -Wreorder -Wregister @gol -Weffc++ -Wstrict-null-sentinel -Wtemplates @gol @@ -3131,6 +3132,31 @@ The compiler rearranges the member initializers for @code{i} and @code{j} to match the declaration order of the members, emitting a warning to that effect. This warning is enabled by @option{-Wall}. +@item -Wno-pessimizing-move @r{(C++ and Objective-C++ only)} +@opindex Wpessimizing-move +@opindex Wno-pessimizing-move +This warning warns when a call to @code{std::move} prevents copy +elision. A typical scenario when copy elision can occur is when returning in +a function with a class return type, when the expression being returned is the +name of a non-volatile automatic object, and is not a function parameter, and +has the same type as the function return type. + +@smallexample +struct T @{ +@dots{} +@}; +T fn() +@{ + T t; + @dots{} + return std::move (t); +@} +@end smallexample + +But in this example, the @code{std::move} call prevents copy elision. + +This warning is enabled by @option{-Wall}. + @item -fext-numeric-literals @r{(C++ and Objective-C++ only)} @opindex fext-numeric-literals @opindex fno-ext-numeric-literals @@ -4036,6 +4062,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wnonnull-compare @gol -Wopenmp-simd @gol -Wparentheses @gol +-Wpessimizing-move @r{(only for C++)} @gol -Wpointer-sign @gol -Wreorder @gol -Wrestrict @gol diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C index e69de29bb2d..858bed6065e 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C @@ -0,0 +1,132 @@ +// PR c++/86981 +// { 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 T { + T() { } + T(const T&) { } + T(T&&) { } +}; +struct U { + U() { } + U(const U&) { } + U(U&&) { } + U(T) { } +}; + +T g; + +T +fn1 () +{ + T t; + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn2 () +{ + // Not a local variable. + return std::move (g); +} + +int +fn3 () +{ + int i = 42; + // Not a class type. + return std::move (i); +} + +T +fn4 (bool b) +{ + T t; + if (b) + throw std::move (t); + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn5 (T t) +{ + // Function parameter; std::move is redundant but not pessimizing. + return std::move (t); +} + +U +fn6 (T t, U u, bool b) +{ + if (b) + return std::move (t); + else + // Function parameter; std::move is redundant but not pessimizing. + return std::move (u); +} + +U +fn6 (bool b) +{ + T t; + U u; + if (b) + return std::move (t); + else + return std::move (u); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn7 () +{ + static T t; + // Non-local; don't warn. + return std::move (t); +} + +T +fn8 () +{ + return T(); +} + +T +fn9 (int i) +{ + T t; + + switch (i) + { + case 1: + return std::move ((t)); // { dg-warning "moving a local object in a return statement prevents copy elision" } + case 2: + return (std::move (t)); // { dg-warning "moving a local object in a return statement prevents copy elision" } + default: + return (std::move ((t))); // { dg-warning "moving a local object in a return statement prevents copy elision" } + } +} + +int +fn10 () +{ + return std::move (42); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C index e69de29bb2d..0ee6e0535dc 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C @@ -0,0 +1,14 @@ +// PR c++/86981 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpessimizing-move" } + +#include <string> +#include <tuple> +#include <utility> + +std::tuple<std::string, std::string> +foo () +{ + std::pair<std::string, std::string> p; + return std::move (p); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C index e69de29bb2d..a1af1230b68 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C @@ -0,0 +1,59 @@ +// PR c++/86981 +// { 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 T { }; +struct U { U(T); }; + +template<typename Tp> +T +fn1 () +{ + T t; + // Non-dependent type. + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template<typename Tp1, typename Tp2> +Tp1 +fn2 () +{ + Tp2 t; + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template<typename Tp1, typename Tp2> +Tp1 +fn3 () +{ + Tp2 t; + return std::move (t); +} + +int +main () +{ + fn1<T>(); + fn2<T, T>(); + fn3<U, T>(); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C index e69de29bb2d..59e148e9f91 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C @@ -0,0 +1,46 @@ +// PR c++/86981 +// { 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 T { }; + +T +fn1 () +{ + T t; + return (1, std::move (t)); +} + +T +fn2 () +{ + T t; + return [&](){ return std::move (t); }(); +} + +T +fn3 () +{ + T t; + return [=](){ return std::move (t); }(); +} diff --git gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C index e69de29bb2d..59741889707 100644 --- gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C +++ gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C @@ -0,0 +1,18 @@ +// PR c++/86981 +// { dg-options "-Wpessimizing-move -std=c++17" } + +#include <utility> +#include <optional> + +struct T { + T() { } + T(const T&) { } + T(T&&) { } +}; + +std::optional<T> +fn () +{ + T t; + return std::move (t); +}
On Mon, 2018-08-20 at 18:54 -0400, Marek Polacek wrote: > On Mon, Aug 20, 2018 at 05:18:49PM -0400, David Malcolm wrote: > > As of r263675 it's now possible to tell the diagnostics subsystem > > that > > the warning and note are related by using an auto_diagnostic_group > > instance [1], so please can this read: > > > > if (can_do_nrvo_p (arg, functype)) > > { > > auto_diagnostic_group d; > > if (warning (OPT_Wpessimizing_move, "moving a local > > object " > > "in a return statement prevents copy > > elision")) > > inform (input_location, "remove %<std::move%> > > call"); > > } > > Sure: > > Bootstrapped/regtested on x86_64-linux, ok for trunk? Thanks! The changed part looks good, but I can't really speak to the rest of the patch. (BTW, could we emit a fix-it hint as part of that "inform" call? How good is the location information at this point?) > 2018-08-20 Marek Polacek <polacek@redhat.com> > > PR c++/86981, Implement -Wpessimizing-move. > * c.opt (Wpessimizing-move): New option. > > * typeck.c (decl_in_std_namespace_p): New. > (is_std_move_p): New. > (can_do_nrvo_p): New, factored out of ... > (check_return_expr): ... here. Warn about potentially harmful > std::move in a return statement. > > * doc/invoke.texi: Document -Wpessimizing-move. > > * g++.dg/cpp0x/Wpessimizing-move1.C: New test. > * g++.dg/cpp0x/Wpessimizing-move2.C: New test. > * g++.dg/cpp0x/Wpessimizing-move3.C: New test. > * g++.dg/cpp0x/Wpessimizing-move4.C: New test. > * g++.dg/cpp1z/Wpessimizing-move1.C: New test. > > diff --git gcc/c-family/c.opt gcc/c-family/c.opt > index 9980bfac11c..76840dd77ad 100644 > --- gcc/c-family/c.opt > +++ gcc/c-family/c.opt > @@ -937,6 +937,10 @@ Wpedantic > C ObjC C++ ObjC++ CPP(cpp_pedantic) CppReason(CPP_W_PEDANTIC) > Warning > ; Documented in common.opt > > +Wpessimizing-move > +C++ ObjC++ Var(warn_pessimizing_move) Warning LangEnabledBy(C++ > ObjC++, Wall) > +Warn about calling std::move on a local object in a return statement > preventing copy elision. > + > Wpmf-conversions > C++ ObjC++ Var(warn_pmf2ptr) Init(1) Warning > Warn when converting the type of pointers to member functions. > diff --git gcc/cp/typeck.c gcc/cp/typeck.c > index 8c13ae9b19b..5fe47299772 100644 > --- gcc/cp/typeck.c > +++ gcc/cp/typeck.c > @@ -9113,6 +9113,58 @@ maybe_warn_about_returning_address_of_local > (tree retval) > return false; > } > > +/* Returns true if DECL is in the std namespace. */ > + > +static bool > +decl_in_std_namespace_p (tree decl) > +{ > + return (decl != NULL_TREE > + && DECL_NAMESPACE_STD_P (decl_namespace_context (decl))); > +} > + > +/* Returns true if FN, a CALL_EXPR, is a call to std::move. */ > + > +static bool > +is_std_move_p (tree fn) > +{ > + /* std::move only takes one argument. */ > + if (call_expr_nargs (fn) != 1) > + return false; > + > + tree fndecl = cp_get_callee_fndecl_nofold (fn); > + if (!decl_in_std_namespace_p (fndecl)) > + return false; > + > + tree name = DECL_NAME (fndecl); > + return name && id_equal (name, "move"); > +} > + > +/* Returns true if RETVAL is a good candidate for the NRVO as per > + [class.copy.elision]. FUNCTYPE is the type the function is > declared > + to return. */ > + > +static bool > +can_do_nrvo_p (tree retval, tree functype) > +{ > + tree result = DECL_RESULT (current_function_decl); > + return (retval != NULL_TREE > + && !processing_template_decl > + /* Must be a local, automatic variable. */ > + && VAR_P (retval) > + && DECL_CONTEXT (retval) == current_function_decl > + && !TREE_STATIC (retval) > + /* And not a lambda or anonymous union proxy. */ > + && !DECL_HAS_VALUE_EXPR_P (retval) > + && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) > + /* 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))) > + /* And the returned value must be non-volatile. */ > + && !TYPE_VOLATILE (TREE_TYPE (retval))); > +} > + > /* Check that returning RETVAL from the current function is valid. > Return an expression explicitly showing all conversions required > to > change RETVAL into the function return type, and to assign it to > @@ -9130,7 +9182,6 @@ check_return_expr (tree retval, bool > *no_warning) > the declared type is incomplete. */ > tree functype; > int fn_returns_value_p; > - bool named_return_value_okay_p; > > *no_warning = false; > > @@ -9342,24 +9393,7 @@ check_return_expr (tree retval, bool > *no_warning) > > See finish_function and finalize_nrv for the rest of this > optimization. */ > > - named_return_value_okay_p = > - (retval != NULL_TREE > - && !processing_template_decl > - /* Must be a local, automatic variable. */ > - && VAR_P (retval) > - && DECL_CONTEXT (retval) == current_function_decl > - && ! TREE_STATIC (retval) > - /* And not a lambda or anonymous union proxy. */ > - && !DECL_HAS_VALUE_EXPR_P (retval) > - && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) > - /* 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))) > - /* And the returned value must be non-volatile. */ > - && ! TYPE_VOLATILE (TREE_TYPE (retval))); > - > + bool named_return_value_okay_p = can_do_nrvo_p (retval, functype); > if (fn_returns_value_p && flag_elide_constructors) > { > if (named_return_value_okay_p > @@ -9375,6 +9409,35 @@ check_return_expr (tree retval, bool > *no_warning) > if (!retval) > return NULL_TREE; > > + /* Warn about wrong usage of std::move in a return statement. */ > + if (!named_return_value_okay_p > + && warn_pessimizing_move > + /* C++98 doesn't know move. */ > + && (cxx_dialect >= cxx11) > + /* This is only interesting for class type. */ > + && CLASS_TYPE_P (functype) > + /* We're looking for *std::move<T&> (&arg). */ > + && REFERENCE_REF_P (retval) > + && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR) > + { > + tree fn = TREE_OPERAND (retval, 0); > + if (is_std_move_p (fn)) > + { > + tree arg = CALL_EXPR_ARG (fn, 0); > + STRIP_NOPS (arg); > + if (TREE_CODE (arg) == ADDR_EXPR) > + arg = TREE_OPERAND (arg, 0); > + /* Warn if we could do copy elision were it not for the > move. */ > + if (can_do_nrvo_p (arg, functype)) > + { > + auto_diagnostic_group d; > + if (warning (OPT_Wpessimizing_move, "moving a local > object " > + "in a return statement prevents copy > elision")) > + inform (input_location, "remove %<std::move%> > call"); > + } > + } > + } > + > /* Do any required conversions. */ > if (retval == result || DECL_CONSTRUCTOR_P > (current_function_decl)) > /* No conversions are required. */ > diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi > index f8287153be1..7a79ba32fc7 100644 > --- gcc/doc/invoke.texi > +++ gcc/doc/invoke.texi > @@ -231,6 +231,7 @@ in the following sections. > -Wdelete-non-virtual-dtor -Wdeprecated-copy -Wliteral-suffix @gol > -Wmultiple-inheritance @gol > -Wnamespaces -Wnarrowing @gol > +-Wpessimizing-move @gol > -Wnoexcept -Wnoexcept-type -Wclass-memaccess @gol > -Wnon-virtual-dtor -Wreorder -Wregister @gol > -Weffc++ -Wstrict-null-sentinel -Wtemplates @gol > @@ -3131,6 +3132,31 @@ The compiler rearranges the member > initializers for @code{i} > and @code{j} to match the declaration order of the members, emitting > a warning to that effect. This warning is enabled by @option{- > Wall}. > > +@item -Wno-pessimizing-move @r{(C++ and Objective-C++ only)} > +@opindex Wpessimizing-move > +@opindex Wno-pessimizing-move > +This warning warns when a call to @code{std::move} prevents copy > +elision. A typical scenario when copy elision can occur is when > returning in > +a function with a class return type, when the expression being > returned is the > +name of a non-volatile automatic object, and is not a function > parameter, and > +has the same type as the function return type. > + > +@smallexample > +struct T @{ > +@dots{} > +@}; > +T fn() > +@{ > + T t; > + @dots{} > + return std::move (t); > +@} > +@end smallexample > + > +But in this example, the @code{std::move} call prevents copy > elision. > + > +This warning is enabled by @option{-Wall}. > + > @item -fext-numeric-literals @r{(C++ and Objective-C++ only)} > @opindex fext-numeric-literals > @opindex fno-ext-numeric-literals > @@ -4036,6 +4062,7 @@ Options} and @ref{Objective-C and Objective-C++ > Dialect Options}. > -Wnonnull-compare @gol > -Wopenmp-simd @gol > -Wparentheses @gol > +-Wpessimizing-move @r{(only for C++)} @gol > -Wpointer-sign @gol > -Wreorder @gol > -Wrestrict @gol > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > index e69de29bb2d..858bed6065e 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > @@ -0,0 +1,132 @@ > +// PR c++/86981 > +// { 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 T { > + T() { } > + T(const T&) { } > + T(T&&) { } > +}; > +struct U { > + U() { } > + U(const U&) { } > + U(U&&) { } > + U(T) { } > +}; > + > +T g; > + > +T > +fn1 () > +{ > + T t; > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +T > +fn2 () > +{ > + // Not a local variable. > + return std::move (g); > +} > + > +int > +fn3 () > +{ > + int i = 42; > + // Not a class type. > + return std::move (i); > +} > + > +T > +fn4 (bool b) > +{ > + T t; > + if (b) > + throw std::move (t); > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +T > +fn5 (T t) > +{ > + // Function parameter; std::move is redundant but not pessimizing. > + return std::move (t); > +} > + > +U > +fn6 (T t, U u, bool b) > +{ > + if (b) > + return std::move (t); > + else > + // Function parameter; std::move is redundant but not > pessimizing. > + return std::move (u); > +} > + > +U > +fn6 (bool b) > +{ > + T t; > + U u; > + if (b) > + return std::move (t); > + else > + return std::move (u); // { dg-warning "moving a local object in > a return statement prevents copy elision" } > +} > + > +T > +fn7 () > +{ > + static T t; > + // Non-local; don't warn. > + return std::move (t); > +} > + > +T > +fn8 () > +{ > + return T(); > +} > + > +T > +fn9 (int i) > +{ > + T t; > + > + switch (i) > + { > + case 1: > + return std::move ((t)); // { dg-warning "moving a local object > in a return statement prevents copy elision" } > + case 2: > + return (std::move (t)); // { dg-warning "moving a local object > in a return statement prevents copy elision" } > + default: > + return (std::move ((t))); // { dg-warning "moving a local > object in a return statement prevents copy elision" } > + } > +} > + > +int > +fn10 () > +{ > + return std::move (42); > +} > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > index e69de29bb2d..0ee6e0535dc 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > @@ -0,0 +1,14 @@ > +// PR c++/86981 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wpessimizing-move" } > + > +#include <string> > +#include <tuple> > +#include <utility> > + > +std::tuple<std::string, std::string> > +foo () > +{ > + std::pair<std::string, std::string> p; > + return std::move (p); > +} > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > index e69de29bb2d..a1af1230b68 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > @@ -0,0 +1,59 @@ > +// PR c++/86981 > +// { 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 T { }; > +struct U { U(T); }; > + > +template<typename Tp> > +T > +fn1 () > +{ > + T t; > + // Non-dependent type. > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +template<typename Tp1, typename Tp2> > +Tp1 > +fn2 () > +{ > + Tp2 t; > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +template<typename Tp1, typename Tp2> > +Tp1 > +fn3 () > +{ > + Tp2 t; > + return std::move (t); > +} > + > +int > +main () > +{ > + fn1<T>(); > + fn2<T, T>(); > + fn3<U, T>(); > +} > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > index e69de29bb2d..59e148e9f91 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > @@ -0,0 +1,46 @@ > +// PR c++/86981 > +// { 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 T { }; > + > +T > +fn1 () > +{ > + T t; > + return (1, std::move (t)); > +} > + > +T > +fn2 () > +{ > + T t; > + return [&](){ return std::move (t); }(); > +} > + > +T > +fn3 () > +{ > + T t; > + return [=](){ return std::move (t); }(); > +} > diff --git gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > index e69de29bb2d..59741889707 100644 > --- gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > +++ gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > @@ -0,0 +1,18 @@ > +// PR c++/86981 > +// { dg-options "-Wpessimizing-move -std=c++17" } > + > +#include <utility> > +#include <optional> > + > +struct T { > + T() { } > + T(const T&) { } > + T(T&&) { } > +}; > + > +std::optional<T> > +fn () > +{ > + T t; > + return std::move (t); > +}
On Tue, Aug 21, 2018, 9:08 AM Marek Polacek <polacek@redhat.com> wrote: > This patch implements -Wpessimizing-move, a C++-specific warning that warns > when using std::move in a return statement precludes the NRVO. Consider: > > struct T { }; > > T f() > { > T t; > return std::move(t); > } > > where we could elide the copy were it not for the move call; the standard > requires that the expression be the name of a non-volatile automatic > object, so > no function call would work there. > Had 't' been a parameter, the move would have been merely redundant, but > that's > for another warning, -Wredundant-move, which should be a fairly easy > extension > of this one. > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > 2018-08-20 Marek Polacek <polacek@redhat.com> > > PR c++/86981, Implement -Wpessimizing-move. > * c.opt (Wpessimizing-move): New option. > > * typeck.c (decl_in_std_namespace_p): New. > (is_std_move_p): New. > (can_do_nrvo_p): New, factored out of ... > (check_return_expr): ... here. Warn about potentially harmful > std::move in a return statement. > > * doc/invoke.texi: Document -Wpessimizing-move. > > * g++.dg/cpp0x/Wpessimizing-move1.C: New test. > * g++.dg/cpp0x/Wpessimizing-move2.C: New test. > * g++.dg/cpp0x/Wpessimizing-move3.C: New test. > * g++.dg/cpp0x/Wpessimizing-move4.C: New test. > * g++.dg/cpp1z/Wpessimizing-move1.C: New test. > > diff --git gcc/c-family/c.opt gcc/c-family/c.opt > index 9980bfac11c..76840dd77ad 100644 > --- gcc/c-family/c.opt > +++ gcc/c-family/c.opt > @@ -937,6 +937,10 @@ Wpedantic > C ObjC C++ ObjC++ CPP(cpp_pedantic) CppReason(CPP_W_PEDANTIC) Warning > ; Documented in common.opt > > +Wpessimizing-move > +C++ ObjC++ Var(warn_pessimizing_move) Warning LangEnabledBy(C++ ObjC++, > Wall) > +Warn about calling std::move on a local object in a return statement > preventing copy elision. > + > Wpmf-conversions > C++ ObjC++ Var(warn_pmf2ptr) Init(1) Warning > Warn when converting the type of pointers to member functions. > diff --git gcc/cp/typeck.c gcc/cp/typeck.c > index 8c13ae9b19b..e7504d5a246 100644 > --- gcc/cp/typeck.c > +++ gcc/cp/typeck.c > @@ -9113,6 +9113,58 @@ maybe_warn_about_returning_address_of_local (tree > retval) > return false; > } > > +/* Returns true if DECL is in the std namespace. */ > + > +static bool > +decl_in_std_namespace_p (tree decl) > +{ > + return (decl != NULL_TREE > + && DECL_NAMESPACE_STD_P (decl_namespace_context (decl))); > +} > + > +/* Returns true if FN, a CALL_EXPR, is a call to std::move. */ > + > +static bool > +is_std_move_p (tree fn) > +{ > + /* std::move only takes one argument. */ > + if (call_expr_nargs (fn) != 1) > + return false; > + > + tree fndecl = cp_get_callee_fndecl_nofold (fn); > + if (!decl_in_std_namespace_p (fndecl)) > + return false; > + > + tree name = DECL_NAME (fndecl); > + return name && id_equal (name, "move"); > +} > + > +/* Returns true if RETVAL is a good candidate for the NRVO as per > + [class.copy.elision]. FUNCTYPE is the type the function is declared > + to return. */ > + > +static bool > +can_do_nrvo_p (tree retval, tree functype) > +{ > + tree result = DECL_RESULT (current_function_decl); > + return (retval != NULL_TREE > + && !processing_template_decl > + /* Must be a local, automatic variable. */ > + && VAR_P (retval) > + && DECL_CONTEXT (retval) == current_function_decl > + && !TREE_STATIC (retval) > + /* And not a lambda or anonymous union proxy. */ > + && !DECL_HAS_VALUE_EXPR_P (retval) > + && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) > + /* 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))) > + /* And the returned value must be non-volatile. */ > + && !TYPE_VOLATILE (TREE_TYPE (retval))); > +} > + > /* Check that returning RETVAL from the current function is valid. > Return an expression explicitly showing all conversions required to > change RETVAL into the function return type, and to assign it to > @@ -9130,7 +9182,6 @@ check_return_expr (tree retval, bool *no_warning) > the declared type is incomplete. */ > tree functype; > int fn_returns_value_p; > - bool named_return_value_okay_p; > > *no_warning = false; > > @@ -9342,24 +9393,7 @@ check_return_expr (tree retval, bool *no_warning) > > See finish_function and finalize_nrv for the rest of this > optimization. */ > > - named_return_value_okay_p = > - (retval != NULL_TREE > - && !processing_template_decl > - /* Must be a local, automatic variable. */ > - && VAR_P (retval) > - && DECL_CONTEXT (retval) == current_function_decl > - && ! TREE_STATIC (retval) > - /* And not a lambda or anonymous union proxy. */ > - && !DECL_HAS_VALUE_EXPR_P (retval) > - && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) > - /* 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))) > - /* And the returned value must be non-volatile. */ > - && ! TYPE_VOLATILE (TREE_TYPE (retval))); > - > + bool named_return_value_okay_p = can_do_nrvo_p (retval, functype); > if (fn_returns_value_p && flag_elide_constructors) > { > if (named_return_value_okay_p > @@ -9375,6 +9409,32 @@ check_return_expr (tree retval, bool *no_warning) > if (!retval) > return NULL_TREE; > > + /* Warn about wrong usage of std::move in a return statement. */ > + if (!named_return_value_okay_p > + && warn_pessimizing_move > + /* C++98 doesn't know move. */ > + && (cxx_dialect >= cxx11) > + /* This is only interesting for class type. */ > + && CLASS_TYPE_P (functype) > + /* We're looking for *std::move<T&> (&arg). */ > + && REFERENCE_REF_P (retval) > + && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR) > + { > + tree fn = TREE_OPERAND (retval, 0); > + if (is_std_move_p (fn)) > + { > + tree arg = CALL_EXPR_ARG (fn, 0); > + STRIP_NOPS (arg); > + if (TREE_CODE (arg) == ADDR_EXPR) > + arg = TREE_OPERAND (arg, 0); > + /* Warn if we could do copy elision were it not for the move. */ > + if (can_do_nrvo_p (arg, functype) > + && warning (OPT_Wpessimizing_move, "moving a local object " > + "in a return statement prevents copy elision")) > + inform (input_location, "remove %<std::move%> call"); > + } > + } > + > Let's factor this into a maybe_warn_pessimizing_move function. Ok with that change. /* Do any required conversions. */ > if (retval == result || DECL_CONSTRUCTOR_P (current_function_decl)) > /* No conversions are required. */ > diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi > index f8287153be1..7a79ba32fc7 100644 > --- gcc/doc/invoke.texi > +++ gcc/doc/invoke.texi > @@ -231,6 +231,7 @@ in the following sections. > -Wdelete-non-virtual-dtor -Wdeprecated-copy -Wliteral-suffix @gol > -Wmultiple-inheritance @gol > -Wnamespaces -Wnarrowing @gol > +-Wpessimizing-move @gol > -Wnoexcept -Wnoexcept-type -Wclass-memaccess @gol > -Wnon-virtual-dtor -Wreorder -Wregister @gol > -Weffc++ -Wstrict-null-sentinel -Wtemplates @gol > @@ -3131,6 +3132,31 @@ The compiler rearranges the member initializers for > @code{i} > and @code{j} to match the declaration order of the members, emitting > a warning to that effect. This warning is enabled by @option{-Wall}. > > +@item -Wno-pessimizing-move @r{(C++ and Objective-C++ only)} > +@opindex Wpessimizing-move > +@opindex Wno-pessimizing-move > +This warning warns when a call to @code{std::move} prevents copy > +elision. A typical scenario when copy elision can occur is when > returning in > +a function with a class return type, when the expression being returned > is the > +name of a non-volatile automatic object, and is not a function parameter, > and > +has the same type as the function return type. > + > +@smallexample > +struct T @{ > +@dots{} > +@}; > +T fn() > +@{ > + T t; > + @dots{} > + return std::move (t); > +@} > +@end smallexample > + > +But in this example, the @code{std::move} call prevents copy elision. > + > +This warning is enabled by @option{-Wall}. > + > @item -fext-numeric-literals @r{(C++ and Objective-C++ only)} > @opindex fext-numeric-literals > @opindex fno-ext-numeric-literals > @@ -4036,6 +4062,7 @@ Options} and @ref{Objective-C and Objective-C++ > Dialect Options}. > -Wnonnull-compare @gol > -Wopenmp-simd @gol > -Wparentheses @gol > +-Wpessimizing-move @r{(only for C++)} @gol > -Wpointer-sign @gol > -Wreorder @gol > -Wrestrict @gol > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > index e69de29bb2d..858bed6065e 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C > @@ -0,0 +1,132 @@ > +// PR c++/86981 > +// { 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 T { > + T() { } > + T(const T&) { } > + T(T&&) { } > +}; > +struct U { > + U() { } > + U(const U&) { } > + U(U&&) { } > + U(T) { } > +}; > + > +T g; > + > +T > +fn1 () > +{ > + T t; > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +T > +fn2 () > +{ > + // Not a local variable. > + return std::move (g); > +} > + > +int > +fn3 () > +{ > + int i = 42; > + // Not a class type. > + return std::move (i); > +} > + > +T > +fn4 (bool b) > +{ > + T t; > + if (b) > + throw std::move (t); > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +T > +fn5 (T t) > +{ > + // Function parameter; std::move is redundant but not pessimizing. > + return std::move (t); > +} > + > +U > +fn6 (T t, U u, bool b) > +{ > + if (b) > + return std::move (t); > + else > + // Function parameter; std::move is redundant but not pessimizing. > + return std::move (u); > +} > + > +U > +fn6 (bool b) > +{ > + T t; > + U u; > + if (b) > + return std::move (t); > + else > + return std::move (u); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +T > +fn7 () > +{ > + static T t; > + // Non-local; don't warn. > + return std::move (t); > +} > + > +T > +fn8 () > +{ > + return T(); > +} > + > +T > +fn9 (int i) > +{ > + T t; > + > + switch (i) > + { > + case 1: > + return std::move ((t)); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > + case 2: > + return (std::move (t)); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > + default: > + return (std::move ((t))); // { dg-warning "moving a local object in > a return statement prevents copy elision" } > + } > +} > + > +int > +fn10 () > +{ > + return std::move (42); > +} > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > index e69de29bb2d..0ee6e0535dc 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C > @@ -0,0 +1,14 @@ > +// PR c++/86981 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wpessimizing-move" } > + > +#include <string> > +#include <tuple> > +#include <utility> > + > +std::tuple<std::string, std::string> > +foo () > +{ > + std::pair<std::string, std::string> p; > + return std::move (p); > +} > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > index e69de29bb2d..a1af1230b68 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C > @@ -0,0 +1,59 @@ > +// PR c++/86981 > +// { 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 T { }; > +struct U { U(T); }; > + > +template<typename Tp> > +T > +fn1 () > +{ > + T t; > + // Non-dependent type. > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +template<typename Tp1, typename Tp2> > +Tp1 > +fn2 () > +{ > + Tp2 t; > + return std::move (t); // { dg-warning "moving a local object in a > return statement prevents copy elision" } > +} > + > +template<typename Tp1, typename Tp2> > +Tp1 > +fn3 () > +{ > + Tp2 t; > + return std::move (t); > +} > + > +int > +main () > +{ > + fn1<T>(); > + fn2<T, T>(); > + fn3<U, T>(); > +} > diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > index e69de29bb2d..59e148e9f91 100644 > --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C > @@ -0,0 +1,46 @@ > +// PR c++/86981 > +// { 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 T { }; > + > +T > +fn1 () > +{ > + T t; > + return (1, std::move (t)); > +} > + > +T > +fn2 () > +{ > + T t; > + return [&](){ return std::move (t); }(); > +} > + > +T > +fn3 () > +{ > + T t; > + return [=](){ return std::move (t); }(); > +} > diff --git gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > index e69de29bb2d..59741889707 100644 > --- gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > +++ gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C > @@ -0,0 +1,18 @@ > +// PR c++/86981 > +// { dg-options "-Wpessimizing-move -std=c++17" } > + > +#include <utility> > +#include <optional> > + > +struct T { > + T() { } > + T(const T&) { } > + T(T&&) { } > +}; > + > +std::optional<T> > +fn () > +{ > + T t; > + return std::move (t); > +} >
On Mon, Aug 20, 2018 at 07:42:10PM -0400, David Malcolm wrote: > On Mon, 2018-08-20 at 18:54 -0400, Marek Polacek wrote: > > On Mon, Aug 20, 2018 at 05:18:49PM -0400, David Malcolm wrote: > > > As of r263675 it's now possible to tell the diagnostics subsystem > > > that > > > the warning and note are related by using an auto_diagnostic_group > > > instance [1], so please can this read: > > > > > > if (can_do_nrvo_p (arg, functype)) > > > { > > > auto_diagnostic_group d; > > > if (warning (OPT_Wpessimizing_move, "moving a local > > > object " > > > "in a return statement prevents copy > > > elision")) > > > inform (input_location, "remove %<std::move%> > > > call"); > > > } > > > > Sure: > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > Thanks! The changed part looks good, but I can't really speak to the > rest of the patch. > > (BTW, could we emit a fix-it hint as part of that "inform" call? How > good is the location information at this point?) Thanks, I can actually use location_of (retval) so that we get: Wpessimizing-move1.C:43:20: warning: moving a local object in a return statement prevents copy elision [-Wpessimizing-move] 43 | return std::move (t); | ~~~~~~~~~~^~~ certainly better than what input_location offers. I'll go with that but I'll be touching the code today anyway and I'll look into using a fix-it hint. Marek
On Tue, 2018-08-21 at 10:01 -0400, Marek Polacek wrote: > On Mon, Aug 20, 2018 at 07:42:10PM -0400, David Malcolm wrote: > > On Mon, 2018-08-20 at 18:54 -0400, Marek Polacek wrote: > > > On Mon, Aug 20, 2018 at 05:18:49PM -0400, David Malcolm wrote: > > > > As of r263675 it's now possible to tell the diagnostics > > > > subsystem > > > > that > > > > the warning and note are related by using an > > > > auto_diagnostic_group > > > > instance [1], so please can this read: > > > > > > > > if (can_do_nrvo_p (arg, functype)) > > > > { > > > > auto_diagnostic_group d; > > > > if (warning (OPT_Wpessimizing_move, "moving a > > > > local > > > > object " > > > > "in a return statement prevents copy > > > > elision")) > > > > inform (input_location, "remove %<std::move%> > > > > call"); > > > > } > > > > > > Sure: > > > > > > Bootstrapped/regtested on x86_64-linux, ok for trunk? > > > > Thanks! The changed part looks good, but I can't really speak to > > the > > rest of the patch. > > > > (BTW, could we emit a fix-it hint as part of that "inform" > > call? How > > good is the location information at this point?) > > Thanks, I can actually use location_of (retval) so that we get: > > Wpessimizing-move1.C:43:20: warning: moving a local object in a > return statement prevents copy elision [-Wpessimizing-move] > 43 | return std::move (t); > | ~~~~~~~~~~^~~ > > certainly better than what input_location offers. I'll go with that > but > I'll be touching the code today anyway and I'll look into using a > fix-it > hint. Thanks. Sorry if this is a bit of a digression from the main point of the patch; I see this all as bonus material - maybe as a followup assuming/once the real part of the patch is reviewed. Am I right in thinking that the above code here ought to be simply: return t; If so, then presumably we need the location of the start of parens, so that we can suggest deletion from the start of the "std::move" through to the open paren, and deletion of the close paren. For a testcase for the fix-it hint, it's better to use something with a length > 1 for the argument (to verify the range is correct), so it would be something like: Wpessimizing-move1.C:43:20: note: remove 'std::move' call: 43 | return std::move (foo); | ~~~~~~~~~~^~~~~ | ----------- - (which admittedly isn't the most readable presentation of that data, but maybe that's fixable in diagnostic-show-locus.c).
On Tue, Aug 21, 2018 at 04:41:52PM +1200, Jason Merrill wrote: > Let's factor this into a maybe_warn_pessimizing_move function. Ok with that > change. Thanks for the quick review. Thus: Bootstrapped/regtested on x86_64-linux, applying to trunk. 2018-08-21 Marek Polacek <polacek@redhat.com> PR c++/86981, Implement -Wpessimizing-move. * c.opt (Wpessimizing-move): New option. * typeck.c (decl_in_std_namespace_p): New. (is_std_move_p): New. (maybe_warn_pessimizing_move): New. (can_do_nrvo_p): New, factored out of ... (check_return_expr): ... here. Warn about potentially harmful std::move in a return statement. * doc/invoke.texi: Document -Wpessimizing-move. * g++.dg/cpp0x/Wpessimizing-move1.C: New test. * g++.dg/cpp0x/Wpessimizing-move2.C: New test. * g++.dg/cpp0x/Wpessimizing-move3.C: New test. * g++.dg/cpp0x/Wpessimizing-move4.C: New test. * g++.dg/cpp1z/Wpessimizing-move1.C: New test. diff --git gcc/c-family/c.opt gcc/c-family/c.opt index 9980bfac11c..76840dd77ad 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -937,6 +937,10 @@ Wpedantic C ObjC C++ ObjC++ CPP(cpp_pedantic) CppReason(CPP_W_PEDANTIC) Warning ; Documented in common.opt +Wpessimizing-move +C++ ObjC++ Var(warn_pessimizing_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn about calling std::move on a local object in a return statement preventing copy elision. + Wpmf-conversions C++ ObjC++ Var(warn_pmf2ptr) Init(1) Warning Warn when converting the type of pointers to member functions. diff --git gcc/cp/typeck.c gcc/cp/typeck.c index 99be38ed8f8..122d9dcd4b3 100644 --- gcc/cp/typeck.c +++ gcc/cp/typeck.c @@ -9126,6 +9126,100 @@ maybe_warn_about_returning_address_of_local (tree retval) return false; } +/* Returns true if DECL is in the std namespace. */ + +static bool +decl_in_std_namespace_p (tree decl) +{ + return (decl != NULL_TREE + && DECL_NAMESPACE_STD_P (decl_namespace_context (decl))); +} + +/* Returns true if FN, a CALL_EXPR, is a call to std::move. */ + +static bool +is_std_move_p (tree fn) +{ + /* std::move only takes one argument. */ + if (call_expr_nargs (fn) != 1) + return false; + + tree fndecl = cp_get_callee_fndecl_nofold (fn); + if (!decl_in_std_namespace_p (fndecl)) + return false; + + tree name = DECL_NAME (fndecl); + return name && id_equal (name, "move"); +} + +/* Returns true if RETVAL is a good candidate for the NRVO as per + [class.copy.elision]. FUNCTYPE is the type the function is declared + to return. */ + +static bool +can_do_nrvo_p (tree retval, tree functype) +{ + tree result = DECL_RESULT (current_function_decl); + return (retval != NULL_TREE + && !processing_template_decl + /* Must be a local, automatic variable. */ + && VAR_P (retval) + && DECL_CONTEXT (retval) == current_function_decl + && !TREE_STATIC (retval) + /* And not a lambda or anonymous union proxy. */ + && !DECL_HAS_VALUE_EXPR_P (retval) + && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) + /* 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))) + /* And the returned value must be non-volatile. */ + && !TYPE_VOLATILE (TREE_TYPE (retval))); +} + +/* Warn about wrong usage of std::move in a return statement. RETVAL + is the expression we are returning; FUNCTYPE is the type the function + is declared to return. */ + +static void +maybe_warn_pessimizing_move (tree retval, tree functype) +{ + if (!warn_pessimizing_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + /* This is only interesting for class types. */ + if (!CLASS_TYPE_P (functype)) + return; + + /* We're looking for *std::move<T&> (&arg). */ + if (REFERENCE_REF_P (retval) + && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR) + { + tree fn = TREE_OPERAND (retval, 0); + if (is_std_move_p (fn)) + { + tree arg = CALL_EXPR_ARG (fn, 0); + STRIP_NOPS (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + /* Warn if we could do copy elision were it not for the move. */ + if (can_do_nrvo_p (arg, functype)) + { + auto_diagnostic_group d; + if (warning_at (location_of (retval), OPT_Wpessimizing_move, + "moving a local object in a return statement " + "prevents copy elision")) + inform (location_of (retval), "remove %<std::move%> call"); + } + } + } +} + /* Check that returning RETVAL from the current function is valid. Return an expression explicitly showing all conversions required to change RETVAL into the function return type, and to assign it to @@ -9143,7 +9237,6 @@ check_return_expr (tree retval, bool *no_warning) the declared type is incomplete. */ tree functype; int fn_returns_value_p; - bool named_return_value_okay_p; *no_warning = false; @@ -9355,24 +9448,7 @@ check_return_expr (tree retval, bool *no_warning) See finish_function and finalize_nrv for the rest of this optimization. */ - named_return_value_okay_p = - (retval != NULL_TREE - && !processing_template_decl - /* Must be a local, automatic variable. */ - && VAR_P (retval) - && DECL_CONTEXT (retval) == current_function_decl - && ! TREE_STATIC (retval) - /* And not a lambda or anonymous union proxy. */ - && !DECL_HAS_VALUE_EXPR_P (retval) - && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) - /* 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))) - /* And the returned value must be non-volatile. */ - && ! TYPE_VOLATILE (TREE_TYPE (retval))); - + bool named_return_value_okay_p = can_do_nrvo_p (retval, functype); if (fn_returns_value_p && flag_elide_constructors) { if (named_return_value_okay_p @@ -9388,6 +9464,9 @@ check_return_expr (tree retval, bool *no_warning) if (!retval) return NULL_TREE; + if (!named_return_value_okay_p) + maybe_warn_pessimizing_move (retval, functype); + /* Do any required conversions. */ if (retval == result || DECL_CONSTRUCTOR_P (current_function_decl)) /* No conversions are required. */ diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index 99849ec6467..e4148297a87 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -231,6 +231,7 @@ in the following sections. -Wdelete-non-virtual-dtor -Wdeprecated-copy -Wliteral-suffix @gol -Wmultiple-inheritance @gol -Wnamespaces -Wnarrowing @gol +-Wpessimizing-move @gol -Wnoexcept -Wnoexcept-type -Wclass-memaccess @gol -Wnon-virtual-dtor -Wreorder -Wregister @gol -Weffc++ -Wstrict-null-sentinel -Wtemplates @gol @@ -3132,6 +3133,31 @@ The compiler rearranges the member initializers for @code{i} and @code{j} to match the declaration order of the members, emitting a warning to that effect. This warning is enabled by @option{-Wall}. +@item -Wno-pessimizing-move @r{(C++ and Objective-C++ only)} +@opindex Wpessimizing-move +@opindex Wno-pessimizing-move +This warning warns when a call to @code{std::move} prevents copy +elision. A typical scenario when copy elision can occur is when returning in +a function with a class return type, when the expression being returned is the +name of a non-volatile automatic object, and is not a function parameter, and +has the same type as the function return type. + +@smallexample +struct T @{ +@dots{} +@}; +T fn() +@{ + T t; + @dots{} + return std::move (t); +@} +@end smallexample + +But in this example, the @code{std::move} call prevents copy elision. + +This warning is enabled by @option{-Wall}. + @item -fext-numeric-literals @r{(C++ and Objective-C++ only)} @opindex fext-numeric-literals @opindex fno-ext-numeric-literals @@ -4037,6 +4063,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wnonnull-compare @gol -Wopenmp-simd @gol -Wparentheses @gol +-Wpessimizing-move @r{(only for C++)} @gol -Wpointer-sign @gol -Wreorder @gol -Wrestrict @gol diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C index e69de29bb2d..858bed6065e 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C @@ -0,0 +1,132 @@ +// PR c++/86981 +// { 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 T { + T() { } + T(const T&) { } + T(T&&) { } +}; +struct U { + U() { } + U(const U&) { } + U(U&&) { } + U(T) { } +}; + +T g; + +T +fn1 () +{ + T t; + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn2 () +{ + // Not a local variable. + return std::move (g); +} + +int +fn3 () +{ + int i = 42; + // Not a class type. + return std::move (i); +} + +T +fn4 (bool b) +{ + T t; + if (b) + throw std::move (t); + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn5 (T t) +{ + // Function parameter; std::move is redundant but not pessimizing. + return std::move (t); +} + +U +fn6 (T t, U u, bool b) +{ + if (b) + return std::move (t); + else + // Function parameter; std::move is redundant but not pessimizing. + return std::move (u); +} + +U +fn6 (bool b) +{ + T t; + U u; + if (b) + return std::move (t); + else + return std::move (u); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn7 () +{ + static T t; + // Non-local; don't warn. + return std::move (t); +} + +T +fn8 () +{ + return T(); +} + +T +fn9 (int i) +{ + T t; + + switch (i) + { + case 1: + return std::move ((t)); // { dg-warning "moving a local object in a return statement prevents copy elision" } + case 2: + return (std::move (t)); // { dg-warning "moving a local object in a return statement prevents copy elision" } + default: + return (std::move ((t))); // { dg-warning "moving a local object in a return statement prevents copy elision" } + } +} + +int +fn10 () +{ + return std::move (42); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C index e69de29bb2d..0ee6e0535dc 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C @@ -0,0 +1,14 @@ +// PR c++/86981 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpessimizing-move" } + +#include <string> +#include <tuple> +#include <utility> + +std::tuple<std::string, std::string> +foo () +{ + std::pair<std::string, std::string> p; + return std::move (p); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C index e69de29bb2d..a1af1230b68 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C @@ -0,0 +1,59 @@ +// PR c++/86981 +// { 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 T { }; +struct U { U(T); }; + +template<typename Tp> +T +fn1 () +{ + T t; + // Non-dependent type. + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template<typename Tp1, typename Tp2> +Tp1 +fn2 () +{ + Tp2 t; + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template<typename Tp1, typename Tp2> +Tp1 +fn3 () +{ + Tp2 t; + return std::move (t); +} + +int +main () +{ + fn1<T>(); + fn2<T, T>(); + fn3<U, T>(); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C index e69de29bb2d..59e148e9f91 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C @@ -0,0 +1,46 @@ +// PR c++/86981 +// { 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 T { }; + +T +fn1 () +{ + T t; + return (1, std::move (t)); +} + +T +fn2 () +{ + T t; + return [&](){ return std::move (t); }(); +} + +T +fn3 () +{ + T t; + return [=](){ return std::move (t); }(); +} diff --git gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C index e69de29bb2d..59741889707 100644 --- gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C +++ gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C @@ -0,0 +1,18 @@ +// PR c++/86981 +// { dg-options "-Wpessimizing-move -std=c++17" } + +#include <utility> +#include <optional> + +struct T { + T() { } + T(const T&) { } + T(T&&) { } +}; + +std::optional<T> +fn () +{ + T t; + return std::move (t); +}
diff --git gcc/c-family/c.opt gcc/c-family/c.opt index 9980bfac11c..76840dd77ad 100644 --- gcc/c-family/c.opt +++ gcc/c-family/c.opt @@ -937,6 +937,10 @@ Wpedantic C ObjC C++ ObjC++ CPP(cpp_pedantic) CppReason(CPP_W_PEDANTIC) Warning ; Documented in common.opt +Wpessimizing-move +C++ ObjC++ Var(warn_pessimizing_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn about calling std::move on a local object in a return statement preventing copy elision. + Wpmf-conversions C++ ObjC++ Var(warn_pmf2ptr) Init(1) Warning Warn when converting the type of pointers to member functions. diff --git gcc/cp/typeck.c gcc/cp/typeck.c index 8c13ae9b19b..e7504d5a246 100644 --- gcc/cp/typeck.c +++ gcc/cp/typeck.c @@ -9113,6 +9113,58 @@ maybe_warn_about_returning_address_of_local (tree retval) return false; } +/* Returns true if DECL is in the std namespace. */ + +static bool +decl_in_std_namespace_p (tree decl) +{ + return (decl != NULL_TREE + && DECL_NAMESPACE_STD_P (decl_namespace_context (decl))); +} + +/* Returns true if FN, a CALL_EXPR, is a call to std::move. */ + +static bool +is_std_move_p (tree fn) +{ + /* std::move only takes one argument. */ + if (call_expr_nargs (fn) != 1) + return false; + + tree fndecl = cp_get_callee_fndecl_nofold (fn); + if (!decl_in_std_namespace_p (fndecl)) + return false; + + tree name = DECL_NAME (fndecl); + return name && id_equal (name, "move"); +} + +/* Returns true if RETVAL is a good candidate for the NRVO as per + [class.copy.elision]. FUNCTYPE is the type the function is declared + to return. */ + +static bool +can_do_nrvo_p (tree retval, tree functype) +{ + tree result = DECL_RESULT (current_function_decl); + return (retval != NULL_TREE + && !processing_template_decl + /* Must be a local, automatic variable. */ + && VAR_P (retval) + && DECL_CONTEXT (retval) == current_function_decl + && !TREE_STATIC (retval) + /* And not a lambda or anonymous union proxy. */ + && !DECL_HAS_VALUE_EXPR_P (retval) + && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) + /* 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))) + /* And the returned value must be non-volatile. */ + && !TYPE_VOLATILE (TREE_TYPE (retval))); +} + /* Check that returning RETVAL from the current function is valid. Return an expression explicitly showing all conversions required to change RETVAL into the function return type, and to assign it to @@ -9130,7 +9182,6 @@ check_return_expr (tree retval, bool *no_warning) the declared type is incomplete. */ tree functype; int fn_returns_value_p; - bool named_return_value_okay_p; *no_warning = false; @@ -9342,24 +9393,7 @@ check_return_expr (tree retval, bool *no_warning) See finish_function and finalize_nrv for the rest of this optimization. */ - named_return_value_okay_p = - (retval != NULL_TREE - && !processing_template_decl - /* Must be a local, automatic variable. */ - && VAR_P (retval) - && DECL_CONTEXT (retval) == current_function_decl - && ! TREE_STATIC (retval) - /* And not a lambda or anonymous union proxy. */ - && !DECL_HAS_VALUE_EXPR_P (retval) - && (DECL_ALIGN (retval) <= DECL_ALIGN (result)) - /* 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))) - /* And the returned value must be non-volatile. */ - && ! TYPE_VOLATILE (TREE_TYPE (retval))); - + bool named_return_value_okay_p = can_do_nrvo_p (retval, functype); if (fn_returns_value_p && flag_elide_constructors) { if (named_return_value_okay_p @@ -9375,6 +9409,32 @@ check_return_expr (tree retval, bool *no_warning) if (!retval) return NULL_TREE; + /* Warn about wrong usage of std::move in a return statement. */ + if (!named_return_value_okay_p + && warn_pessimizing_move + /* C++98 doesn't know move. */ + && (cxx_dialect >= cxx11) + /* This is only interesting for class type. */ + && CLASS_TYPE_P (functype) + /* We're looking for *std::move<T&> (&arg). */ + && REFERENCE_REF_P (retval) + && TREE_CODE (TREE_OPERAND (retval, 0)) == CALL_EXPR) + { + tree fn = TREE_OPERAND (retval, 0); + if (is_std_move_p (fn)) + { + tree arg = CALL_EXPR_ARG (fn, 0); + STRIP_NOPS (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + /* Warn if we could do copy elision were it not for the move. */ + if (can_do_nrvo_p (arg, functype) + && warning (OPT_Wpessimizing_move, "moving a local object " + "in a return statement prevents copy elision")) + inform (input_location, "remove %<std::move%> call"); + } + } + /* Do any required conversions. */ if (retval == result || DECL_CONSTRUCTOR_P (current_function_decl)) /* No conversions are required. */ diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi index f8287153be1..7a79ba32fc7 100644 --- gcc/doc/invoke.texi +++ gcc/doc/invoke.texi @@ -231,6 +231,7 @@ in the following sections. -Wdelete-non-virtual-dtor -Wdeprecated-copy -Wliteral-suffix @gol -Wmultiple-inheritance @gol -Wnamespaces -Wnarrowing @gol +-Wpessimizing-move @gol -Wnoexcept -Wnoexcept-type -Wclass-memaccess @gol -Wnon-virtual-dtor -Wreorder -Wregister @gol -Weffc++ -Wstrict-null-sentinel -Wtemplates @gol @@ -3131,6 +3132,31 @@ The compiler rearranges the member initializers for @code{i} and @code{j} to match the declaration order of the members, emitting a warning to that effect. This warning is enabled by @option{-Wall}. +@item -Wno-pessimizing-move @r{(C++ and Objective-C++ only)} +@opindex Wpessimizing-move +@opindex Wno-pessimizing-move +This warning warns when a call to @code{std::move} prevents copy +elision. A typical scenario when copy elision can occur is when returning in +a function with a class return type, when the expression being returned is the +name of a non-volatile automatic object, and is not a function parameter, and +has the same type as the function return type. + +@smallexample +struct T @{ +@dots{} +@}; +T fn() +@{ + T t; + @dots{} + return std::move (t); +@} +@end smallexample + +But in this example, the @code{std::move} call prevents copy elision. + +This warning is enabled by @option{-Wall}. + @item -fext-numeric-literals @r{(C++ and Objective-C++ only)} @opindex fext-numeric-literals @opindex fno-ext-numeric-literals @@ -4036,6 +4062,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wnonnull-compare @gol -Wopenmp-simd @gol -Wparentheses @gol +-Wpessimizing-move @r{(only for C++)} @gol -Wpointer-sign @gol -Wreorder @gol -Wrestrict @gol diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C index e69de29bb2d..858bed6065e 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move1.C @@ -0,0 +1,132 @@ +// PR c++/86981 +// { 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 T { + T() { } + T(const T&) { } + T(T&&) { } +}; +struct U { + U() { } + U(const U&) { } + U(U&&) { } + U(T) { } +}; + +T g; + +T +fn1 () +{ + T t; + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn2 () +{ + // Not a local variable. + return std::move (g); +} + +int +fn3 () +{ + int i = 42; + // Not a class type. + return std::move (i); +} + +T +fn4 (bool b) +{ + T t; + if (b) + throw std::move (t); + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn5 (T t) +{ + // Function parameter; std::move is redundant but not pessimizing. + return std::move (t); +} + +U +fn6 (T t, U u, bool b) +{ + if (b) + return std::move (t); + else + // Function parameter; std::move is redundant but not pessimizing. + return std::move (u); +} + +U +fn6 (bool b) +{ + T t; + U u; + if (b) + return std::move (t); + else + return std::move (u); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +T +fn7 () +{ + static T t; + // Non-local; don't warn. + return std::move (t); +} + +T +fn8 () +{ + return T(); +} + +T +fn9 (int i) +{ + T t; + + switch (i) + { + case 1: + return std::move ((t)); // { dg-warning "moving a local object in a return statement prevents copy elision" } + case 2: + return (std::move (t)); // { dg-warning "moving a local object in a return statement prevents copy elision" } + default: + return (std::move ((t))); // { dg-warning "moving a local object in a return statement prevents copy elision" } + } +} + +int +fn10 () +{ + return std::move (42); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C index e69de29bb2d..0ee6e0535dc 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move2.C @@ -0,0 +1,14 @@ +// PR c++/86981 +// { dg-do compile { target c++11 } } +// { dg-options "-Wpessimizing-move" } + +#include <string> +#include <tuple> +#include <utility> + +std::tuple<std::string, std::string> +foo () +{ + std::pair<std::string, std::string> p; + return std::move (p); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C index e69de29bb2d..a1af1230b68 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move3.C @@ -0,0 +1,59 @@ +// PR c++/86981 +// { 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 T { }; +struct U { U(T); }; + +template<typename Tp> +T +fn1 () +{ + T t; + // Non-dependent type. + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template<typename Tp1, typename Tp2> +Tp1 +fn2 () +{ + Tp2 t; + return std::move (t); // { dg-warning "moving a local object in a return statement prevents copy elision" } +} + +template<typename Tp1, typename Tp2> +Tp1 +fn3 () +{ + Tp2 t; + return std::move (t); +} + +int +main () +{ + fn1<T>(); + fn2<T, T>(); + fn3<U, T>(); +} diff --git gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C index e69de29bb2d..59e148e9f91 100644 --- gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C +++ gcc/testsuite/g++.dg/cpp0x/Wpessimizing-move4.C @@ -0,0 +1,46 @@ +// PR c++/86981 +// { 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 T { }; + +T +fn1 () +{ + T t; + return (1, std::move (t)); +} + +T +fn2 () +{ + T t; + return [&](){ return std::move (t); }(); +} + +T +fn3 () +{ + T t; + return [=](){ return std::move (t); }(); +} diff --git gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C index e69de29bb2d..59741889707 100644 --- gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C +++ gcc/testsuite/g++.dg/cpp1z/Wpessimizing-move1.C @@ -0,0 +1,18 @@ +// PR c++/86981 +// { dg-options "-Wpessimizing-move -std=c++17" } + +#include <utility> +#include <optional> + +struct T { + T() { } + T(const T&) { } + T(T&&) { } +}; + +std::optional<T> +fn () +{ + T t; + return std::move (t); +}
This patch implements -Wpessimizing-move, a C++-specific warning that warns when using std::move in a return statement precludes the NRVO. Consider: struct T { }; T f() { T t; return std::move(t); } where we could elide the copy were it not for the move call; the standard requires that the expression be the name of a non-volatile automatic object, so no function call would work there. Had 't' been a parameter, the move would have been merely redundant, but that's for another warning, -Wredundant-move, which should be a fairly easy extension of this one. Bootstrapped/regtested on x86_64-linux, ok for trunk? 2018-08-20 Marek Polacek <polacek@redhat.com> PR c++/86981, Implement -Wpessimizing-move. * c.opt (Wpessimizing-move): New option. * typeck.c (decl_in_std_namespace_p): New. (is_std_move_p): New. (can_do_nrvo_p): New, factored out of ... (check_return_expr): ... here. Warn about potentially harmful std::move in a return statement. * doc/invoke.texi: Document -Wpessimizing-move. * g++.dg/cpp0x/Wpessimizing-move1.C: New test. * g++.dg/cpp0x/Wpessimizing-move2.C: New test. * g++.dg/cpp0x/Wpessimizing-move3.C: New test. * g++.dg/cpp0x/Wpessimizing-move4.C: New test. * g++.dg/cpp1z/Wpessimizing-move1.C: New test.