This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: C++ PATCH for c++/87029, Implement -Wredundant-move


On Thu, Aug 23, 2018 at 12:12 PM, Marek Polacek <polacek@redhat.com> wrote:
> Now that we have -Wpessimizing-move implemented, it was fairly easy to extend
> it to -Wredundant-move, which warns about redundant calls to std::move; it's
> supposed to detect cases where the compiler is obliged to treat an object as
> if it were an rvalue, so calling std::move is pointless.  Since we implement
> Core Issue 1579, it warns in more situations than clang++.  I described this
> in more detail in the manual entry.
>
> David, I've also tweaked the warning to use fix-it hints; the point is to turn
>
>   return std::move (x);
>
> into
>
>   return x;
>
> I didn't fool around with looking for the locations of the parens; I've used a
> DECL_NAME hack instead.  I've verified it using -fdiagnostics-generate-patch,
> which produces e.g.:
>
> @@ -27,7 +27,7 @@
>  f ()
>  {
>    T foo;
> -  return std::move (foo);
> +  return foo;
>  }
>
> g++.dg/diagnostic/move1.C tests the fix-it hints for -Wredundant-move and
> -Wpessimizing-move.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-08-22  Marek Polacek  <polacek@redhat.com>
>
>         PR c++/87029, Implement -Wredundant-move.
>         * c.opt (Wredundant-move): New option.
>
>         * typeck.c (maybe_warn_pessimizing_move): Call convert_from_reference.
>         Warn about redundant moves.  Use fix-it hints.
>
>         * doc/invoke.texi: Document -Wredundant-move.
>
>         * g++.dg/cpp0x/Wredundant-move1.C: New test.
>         * g++.dg/cpp0x/Wredundant-move2.C: New test.
>         * g++.dg/cpp0x/Wredundant-move3.C: New test.
>         * g++.dg/cpp0x/Wredundant-move4.C: New test.
>         * g++.dg/diagnostic/move1.C: New test.
>
> diff --git gcc/c-family/c.opt gcc/c-family/c.opt
> index 76840dd77ad..a92be089b40 100644
> --- gcc/c-family/c.opt
> +++ gcc/c-family/c.opt
> @@ -985,6 +985,10 @@ Wredundant-decls
>  C ObjC C++ ObjC++ Var(warn_redundant_decls) Warning
>  Warn about multiple declarations of the same object.
>
> +Wredundant-move
> +C++ ObjC++ Var(warn_redundant_move) Warning LangEnabledBy(C++ ObjC++, Wall)
> +Warn about redundant calls to std::move.

I don't think this belongs in -Wall, since the code is only redundant,
not wrong.

>  Wregister
>  C++ ObjC++ Var(warn_register) Warning
>  Warn about uses of register storage specifier.
> diff --git gcc/cp/typeck.c gcc/cp/typeck.c
> index 122d9dcd4b3..3f0b263525e 100644
> --- gcc/cp/typeck.c
> +++ gcc/cp/typeck.c
> @@ -9185,7 +9185,7 @@ can_do_nrvo_p (tree retval, tree functype)
>  static void
>  maybe_warn_pessimizing_move (tree retval, tree functype)
>  {
> -  if (!warn_pessimizing_move)
> +  if (!(warn_pessimizing_move || warn_redundant_move))
>      return;
>
>    /* C++98 doesn't know move.  */
> @@ -9207,14 +9207,31 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
>           STRIP_NOPS (arg);
>           if (TREE_CODE (arg) == ADDR_EXPR)
>             arg = TREE_OPERAND (arg, 0);
> +         arg = convert_from_reference (arg);
>           /* 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");
> +             gcc_rich_location richloc (location_of (retval));
> +             tree name = DECL_NAME (arg);
> +             richloc.add_fixit_replace (IDENTIFIER_POINTER (name));
> +             warning_at (&richloc, OPT_Wpessimizing_move,
> +                         "moving a local object in a return statement "
> +                         "prevents copy elision");
> +           }
> +         /* Warn if the move is redundant.  It is redundant when we would
> +            do maybe-rvalue overload resolution even without std::move.  */
> +         else if (((VAR_P (arg) && !DECL_HAS_VALUE_EXPR_P (arg))
> +                   || TREE_CODE (arg) == PARM_DECL)
> +                  && DECL_CONTEXT (arg) == current_function_decl
> +                  && !TREE_STATIC (arg))

This condition should be shared with check_return_expr rather than copied.

> +           {
> +             auto_diagnostic_group d;
> +             gcc_rich_location richloc (location_of (retval));
> +             tree name = DECL_NAME (arg);
> +             richloc.add_fixit_replace (IDENTIFIER_POINTER (name));
> +             warning_at (&richloc, OPT_Wredundant_move,
> +                         "redundant move in return statement");
>             }
>         }
>      }
> diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
> index e4148297a87..341499f49b7 100644
> --- gcc/doc/invoke.texi
> +++ gcc/doc/invoke.texi
> @@ -231,7 +231,7 @@ in the following sections.
>  -Wdelete-non-virtual-dtor -Wdeprecated-copy  -Wliteral-suffix @gol
>  -Wmultiple-inheritance @gol
>  -Wnamespaces  -Wnarrowing @gol
> --Wpessimizing-move @gol
> +-Wpessimizing-move  -Wredundant-move @gol
>  -Wnoexcept  -Wnoexcept-type  -Wclass-memaccess @gol
>  -Wnon-virtual-dtor  -Wreorder  -Wregister @gol
>  -Weffc++  -Wstrict-null-sentinel  -Wtemplates @gol
> @@ -3158,6 +3158,49 @@ But in this example, the @code{std::move} call prevents copy elision.
>
>  This warning is enabled by @option{-Wall}.
>
> +@item -Wno-redundant-move @r{(C++ and Objective-C++ only)}
> +@opindex Wredundant-move
> +@opindex Wno-redundant-move
> +This warning warns about redundant calls to @code{std::move}; that is, when
> +a move operation would have been performed even without the @code{std::move}
> +call.  This happens because the compiler is forced to treat the object as if
> +it were an rvalue in certain situations such as returning a local variable,
> +where copy elision isn't applicable.  Consider:
> +
> +@smallexample
> +struct T @{
> +@dots{}
> +@};
> +T fn(T t)
> +@{
> +  @dots{}
> +  return std::move (t);
> +@}
> +@end smallexample
> +
> +Here, the @code{std::move} call is redundant.  Because G++ implements Core
> +Issue 1579, another example is:
> +
> +@smallexample
> +struct T @{ // convertible to U
> +@dots{}
> +@};
> +struct U @{
> +@dots{}
> +@};
> +U fn()
> +@{
> +  T t;
> +  @dots{}
> +  return std::move (t);
> +@}
> +@end smallexample
> +In this example, copy elision isn't applicable because the type of the
> +expression being returned and the function return type differ, yet G++
> +treats the return value as if it were designated by an rvalue.
> +
> +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
> @@ -4065,6 +4108,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}.
>  -Wparentheses  @gol
>  -Wpessimizing-move @r{(only for C++)}  @gol
>  -Wpointer-sign  @gol
> +-Wredundant-move @r{(only for C++)}  @gol
>  -Wreorder   @gol
>  -Wrestrict   @gol
>  -Wreturn-type  @gol
> diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
> index e69de29bb2d..0269ffbbc1f 100644
> --- gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
> +++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move1.C
> @@ -0,0 +1,106 @@
> +// PR c++/87029
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wredundant-move" }
> +
> +// Define std::move.
> +namespace std {
> +  template<typename _Tp>
> +    struct remove_reference
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    constexpr typename std::remove_reference<_Tp>::type&&
> +    move(_Tp&& __t) noexcept
> +    { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
> +}
> +
> +struct T {
> +  T() { }
> +  T(const T&) { }
> +  T(T&&) { }
> +};
> +
> +struct U {
> +  U() { }
> +  U(const U&) { }
> +  U(U&&) { }
> +  U(T) { }
> +};
> +
> +T
> +fn1 (T t)
> +{
> +  return t;
> +}
> +
> +T
> +fn2 (T t)
> +{
> +  // Will use move even without std::move.
> +  return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +T
> +fn3 (const T t)
> +{
> +  // t is const: will decay into move.
> +  return t;
> +}
> +
> +T
> +fn4 (const T t)
> +{
> +  // t is const: will decay into move despite std::move, so it's redundant.
> +  return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +int
> +fn5 (int i)
> +{
> +  // Not a class type.
> +  return std::move (i);
> +}
> +
> +T
> +fn6 (T t, bool b)
> +{
> +  if (b)
> +    throw std::move (t);
> +  return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +U
> +fn7 (T t)
> +{
> +  // Core 1579 means we'll get a move here.
> +  return t;
> +}
> +
> +U
> +fn8 (T t)
> +{
> +  // Core 1579 means we'll get a move here.  Even without std::move.
> +  return std::move (t);  // { dg-warning "redundant move in return statement" }
> +}
> +
> +T
> +fn9 (T& t)
> +{
> +  // T is a reference and the move isn't redundant.
> +  return std::move (t);
> +}
> +
> +T
> +fn10 (T&& t)
> +{
> +  // T is a reference and the move isn't redundant.
> +  return std::move (t);
> +}
> diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C
> index e69de29bb2d..f181afeeb84 100644
> --- gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C
> +++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move2.C
> @@ -0,0 +1,57 @@
> +// PR c++/87029
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wredundant-move" }
> +
> +// Define std::move.
> +namespace std {
> +  template<typename _Tp>
> +    struct remove_reference
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    constexpr typename std::remove_reference<_Tp>::type&&
> +    move(_Tp&& __t) noexcept
> +    { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
> +}
> +
> +struct T { };
> +struct U { U(T); };
> +
> +template<typename Tp>
> +T
> +fn1 (T t)
> +{
> +  // Non-dependent type.
> +  return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +template<typename Tp1, typename Tp2>
> +Tp1
> +fn2 (Tp2 t)
> +{
> +  return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +template<typename Tp1, typename Tp2>
> +Tp1
> +fn3 (Tp2 t)
> +{
> +  return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +int
> +main ()
> +{
> +  T t;
> +  fn1<T>(t);
> +  fn2<T, T>(t);
> +  fn3<U, T>(t);
> +}
> diff --git gcc/testsuite/g++.dg/cpp0x/Wredundant-move3.C gcc/testsuite/g++.dg/cpp0x/Wredundant-move3.C
> index e69de29bb2d..7084134e370 100644
> --- gcc/testsuite/g++.dg/cpp0x/Wredundant-move3.C
> +++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move3.C
> @@ -0,0 +1,43 @@
> +// PR c++/87029
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wredundant-move" }
> +
> +// Define std::move.
> +namespace std {
> +  template<typename _Tp>
> +    struct remove_reference
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    constexpr typename std::remove_reference<_Tp>::type&&
> +    move(_Tp&& __t) noexcept
> +    { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
> +}
> +
> +struct 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/cpp0x/Wredundant-move4.C gcc/testsuite/g++.dg/cpp0x/Wredundant-move4.C
> index e69de29bb2d..aa89e46de99 100644
> --- gcc/testsuite/g++.dg/cpp0x/Wredundant-move4.C
> +++ gcc/testsuite/g++.dg/cpp0x/Wredundant-move4.C
> @@ -0,0 +1,86 @@
> +// PR c++/87029
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-Wredundant-move" }
> +
> +// Define std::move.
> +namespace std {
> +  template<typename _Tp>
> +    struct remove_reference
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    struct remove_reference<_Tp&&>
> +    { typedef _Tp   type; };
> +
> +  template<typename _Tp>
> +    constexpr typename std::remove_reference<_Tp>::type&&
> +    move(_Tp&& __t) noexcept
> +    { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); }
> +}
> +
> +struct T {
> +  T() { }
> +  T(const T&) { }
> +  T(T&&) { }
> +};
> +
> +struct U {
> +  U() { }
> +  U(const U&) { }
> +  U(U&&) { }
> +  U(T) { }
> +};
> +
> +U
> +fn1 (T t, bool b)
> +{
> +  if (b)
> +    return t;
> +  else
> +    return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +U
> +fn2 (bool b)
> +{
> +  T t;
> +  if (b)
> +    return t;
> +  else
> +    return std::move (t); // { dg-warning "redundant move in return statement" }
> +}
> +
> +U
> +fn3 (bool b)
> +{
> +  static T t;
> +  if (b)
> +    return t;
> +  else
> +    return std::move (t);
> +}
> +
> +T g;
> +
> +U
> +fn4 (bool b)
> +{
> +  if (b)
> +    return g;
> +  else
> +    return std::move (g);
> +}
> +
> +long int
> +fn5 (bool b)
> +{
> +  int i = 42;
> +  if (b)
> +    return i;
> +  else
> +    return std::move (i);
> +}
> diff --git gcc/testsuite/g++.dg/diagnostic/move1.C gcc/testsuite/g++.dg/diagnostic/move1.C
> index e69de29bb2d..ddf630da843 100644
> --- gcc/testsuite/g++.dg/diagnostic/move1.C
> +++ gcc/testsuite/g++.dg/diagnostic/move1.C
> @@ -0,0 +1,47 @@
> +// { dg-do compile { target c++11 } }
> +// { dg-options "-fdiagnostics-show-caret -Wall" }
> +
> +// 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
> +f1 ()
> +{
> +  T hello;
> +  return std::move (hello); /* { dg-warning "moving a local object" }
> +  { dg-begin-multiline-output "" }
> +   return std::move (hello);
> +          ~~~~~~~~~~^~~~~~~
> +          hello
> +  { dg-end-multiline-output "" } */
> +}
> +
> +T
> +f2 (T David)
> +{
> +  return std::move (David); /* { dg-warning "redundant move" }
> +  { dg-begin-multiline-output "" }
> +   return std::move (David);
> +          ~~~~~~~~~~^~~~~~~
> +          David
> +  { dg-end-multiline-output "" } */
> +}


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]