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 <pola...@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 "" } */
> +}
