C++ PATCH for c++/86981, implement -Wpessimizing-move

Jason Merrill jason@redhat.com
Tue Aug 21 06:00:00 GMT 2018


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



More information about the Gcc-patches mailing list