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]

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


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