On Sat, Aug 25, 2018 at 5:34 AM, Marek Polacek <pola...@redhat.com> wrote:
> On Fri, Aug 24, 2018 at 11:32:18PM +1000, Jason Merrill wrote:
>> On Fri, Aug 24, 2018 at 12:53 AM, Marek Polacek <pola...@redhat.com> wrote:
>> > On Thu, Aug 23, 2018 at 10:44:30AM -0400, Marek Polacek wrote:
>> >> +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" }
>> >> +}
>> >
>> > This should read "decay into copy". We can't move from a const object.
>> > Consider
>> > it fixed.
>>
>> Well, we'll do the overload resolution as though t were an rvalue,
>> even though it's const; it's just unlikely to succeed, since a
>> constructor taking a const rvalue reference doesn't make much sense.
>
> Ah, true. I guess that's only used in like std::reference_wrapper.
>
>> It occurs to me that the std::move might not be redundant in some
>> cases: for the implicit treatment as an rvalue, the return must select
>> a constructor that takes an rvalue reference to the returned object's
>> type. With an explict std::move, that restriction doesn't apply. So,
>> for
>>
>> struct C { };
>> struct A {
>> operator C() &;
>> operator C() &&;
>> };
>>
>> C f(A a)
>> {
>> return a; // calls operator C()&
>> return std::move(a); // calls operator C()&&
>> }
>>
>> ...though I see we currently get the first return wrong, and call the
>> rvalue overload for both. I think there was a recent core issue in
>> this area, I'll try to find that later.
>
> You're right. Wow, I did not think of this. I went looking for that DR but
> I'm not finding it, please do let me know if you find it. Does that mean that
> treat_lvalue_as_rvalue_p will have to change?
No, the problem is with the LOOKUP_PREFER_RVALUE block in
build_over_call, which fails to consider "the object's type".
> Do you want me to open a PR?
Please.
>> Anyway, I imagine this sort of example is rare enough that the warning
>> is still worth having; people doing this can use static_cast instead
>> or just turn off the warning.
>>
>> BTW, Instead of using location_of (retval) in each diagnostic call,
>> let's put at the top of the function
>>
>> location_t loc = cp_expr_loc_or_loc (retval, input_location);
>>
>> and use 'loc' in the diagnostics. This is the pattern I generally mean to
>> use.
>
> Done.
>
> Bootstrapped/regtested on x86_64-linux, ok for trunk?
>
> 2018-08-24 Marek Polacek <pola...@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..cd1ee224883 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,9 @@ can_do_nrvo_p (tree retval, tree functype)
> static void
> maybe_warn_pessimizing_move (tree retval, tree functype)
> {
> - if (!warn_pessimizing_move)
> + location_t loc = cp_expr_loc_or_loc (retval, input_location);
> +
> + if (!(warn_pessimizing_move || warn_redundant_move))
> return;
Let's actually set loc after the early return. OK with that change.
> /* C++98 doesn't know move. */
> @@ -9207,14 +9222,24 @@ 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,
> + if (warning_at (loc, OPT_Wpessimizing_move,
> "moving a local object in a return statement "
> "prevents copy elision"))
> - inform (location_of (retval), "remove %<std::move%> call");
> + inform (loc, "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 (loc, OPT_Wredundant_move,
> + "redundant move in return statement"))
> + inform (loc, "remove %<std::move%> call");
> }
> }
> }
> @@ -9494,11 +9519,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..5d4a25dbc3b 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 copy.
> + return t;
> +}
> +
> +T
> +fn4 (const T t)
> +{
> + // t is const: will decay into copy 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);
> +}
