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 Wed, 2018-08-22 at 20:12 -0400, Marek Polacek 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.

Thanks (and ideally, we'd have a precanned way for DejaGnu to test that
the fix-its hints work, and lead to code that resolves the diagnostics;
I've toyed around with doing so, but it's fiddly, involving a second
compile; ugh).

Regarding the fix-it hint code: I'm not convinced that it works in
every possible scenario.

Consider:

(A):

  namespace ns {
     int some_global;
  };

  int fn ()
  {
    return std::move (ns::some_global);
  }

which I believe will generate this bogus suggestion:

    return std::move (ns::some_global);
           ~~~~~~~~~~^~~~~~~~~~~~~~~~~
           some_global

since IIRC, DECL_NAME doesn't contain any explicit namespace for the
way the var was referred to.

(B):

#ifdef SOME_CONFIG_FLAG
# define CONFIGURY_GLOBAL global_a
#else
# define CONFIGURY_GLOBAL global_b
#endif

  int fn ()
  {
     return std::move (CONFIGURY_GLOBAL);
  }

which presumably will erroneously strip out the macro in the fix-it
hint:

     return std::move (CONFIGURY_GLOBAL);
            ~~~~~~~~~~^~~~~~~~~~~~~~~~~~
            global_a

rich_location will discard fix-it hints for locations that are in macro
expansions to try to avoid problems like this, but (B)'s location will 
be a non-macro location, I believe.

Hence my suggestion to, if possible, use the locations of the parens,
so that the fix-it hint preserves the user's spelling of the variable.
 
But I appreciate that it might be unreasonably difficult to get at
those locations.  Hence, I think the fix-it hint part of the patch is
probably good enough as-is: I'd rather not let "perfect be the enemy of
the good" here.

Dave

> 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.
> +
>  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))
> +	    {
> +	      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]