C++ PATCH for c++/87029, Implement -Wredundant-move
Marek Polacek
polacek@redhat.com
Thu Aug 23 14:44:00 GMT 2018
On Thu, Aug 23, 2018 at 11:59:13PM +1200, Jason Merrill wrote:
> On Thu, Aug 23, 2018 at 12:12 PM, Marek Polacek <polacek@redhat.com> wrote:
> > +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.
Hmm, OK.
> > + /* 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.
Done. I'm puzzled why I didn't do it before as I'd planned.
I've also dropped the fix-it hints as explained here:
<https://gcc.gnu.org/ml/gcc-patches/2018-08/msg01479.html>
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2018-08-23 Marek Polacek <polacek@redhat.com>
PR c++/87029, Implement -Wredundant-move.
* c.opt (Wredundant-move): New option.
* typeck.c (treat_lvalue_as_rvalue_p): New function.
(maybe_warn_pessimizing_move): Call convert_from_reference.
Warn about redundant moves.
* 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.
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 76840dd77ad..31a2b972919 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++,Wextra)
+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..2d556faafc4 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -9178,6 +9178,19 @@ can_do_nrvo_p (tree retval, tree functype)
&& !TYPE_VOLATILE (TREE_TYPE (retval)));
}
+/* Returns true if we should treat RETVAL, an expression being returned,
+ as if it were designated by an rvalue. See [class.copy.elision]. */
+
+static bool
+treat_lvalue_as_rvalue_p (tree retval)
+{
+ return ((cxx_dialect != cxx98)
+ && ((VAR_P (retval) && !DECL_HAS_VALUE_EXPR_P (retval))
+ || TREE_CODE (retval) == PARM_DECL)
+ && DECL_CONTEXT (retval) == current_function_decl
+ && !TREE_STATIC (retval));
+}
+
/* Warn about wrong usage of std::move in a return statement. RETVAL
is the expression we are returning; FUNCTYPE is the type the function
is declared to return. */
@@ -9185,7 +9198,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,6 +9220,7 @@ 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))
{
@@ -9216,6 +9230,15 @@ maybe_warn_pessimizing_move (tree retval, tree functype)
"prevents copy elision"))
inform (location_of (retval), "remove %<std::move%> call");
}
+ /* Warn if the move is redundant. It is redundant when we would
+ do maybe-rvalue overload resolution even without std::move. */
+ else if (treat_lvalue_as_rvalue_p (arg))
+ {
+ auto_diagnostic_group d;
+ if (warning_at (location_of (retval), OPT_Wredundant_move,
+ "redundant move in return statement"))
+ inform (location_of (retval), "remove %<std::move%> call");
+ }
}
}
}
@@ -9494,11 +9517,7 @@ check_return_expr (tree retval, bool *no_warning)
Note that these conditions are similar to, but not as strict as,
the conditions for the named return value optimization. */
bool converted = false;
- if ((cxx_dialect != cxx98)
- && ((VAR_P (retval) && !DECL_HAS_VALUE_EXPR_P (retval))
- || TREE_CODE (retval) == PARM_DECL)
- && DECL_CONTEXT (retval) == current_function_decl
- && !TREE_STATIC (retval)
+ if (treat_lvalue_as_rvalue_p (retval)
/* This is only interesting for class type. */
&& CLASS_TYPE_P (functype))
{
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index e4148297a87..985ef9f3510 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{-Wextra}.
+
@item -fext-numeric-literals @r{(C++ and Objective-C++ only)}
@opindex fext-numeric-literals
@opindex fno-ext-numeric-literals
@@ -4112,6 +4155,7 @@ name is still supported, but the newer name is more descriptive.)
-Wold-style-declaration @r{(C only)} @gol
-Woverride-init @gol
-Wsign-compare @r{(C only)} @gol
+-Wredundant-move @r{(only for C++)} @gol
-Wtype-limits @gol
-Wuninitialized @gol
-Wshift-negative-value @r{(in C++03 and in C99 and newer)} @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);
+}
More information about the Gcc-patches
mailing list