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 <pola...@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);
> +}
