* [PATCH] c++: Implement -Wself-move warning [PR81159] @ 2022-08-09 16:37 Marek Polacek 2022-08-15 19:54 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-08-09 16:37 UTC (permalink / raw) To: GCC Patches, Jason Merrill About 5 years ago we got a request to implement -Wself-move, which warns about useless moves like this: int x; x = std::move (x); This patch implements that warning. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? PR c++/81159 gcc/c-family/ChangeLog: * c.opt (Wself-move): New option. gcc/cp/ChangeLog: * typeck.cc (maybe_warn_self_move): New. (cp_build_modify_expr): Call maybe_warn_self_move. gcc/ChangeLog: * doc/invoke.texi: Document -Wself-move. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move1.C: New test. --- gcc/c-family/c.opt | 4 ++ gcc/cp/typeck.cc | 48 +++++++++++++- gcc/doc/invoke.texi | 23 ++++++- gcc/testsuite/g++.dg/warn/Wself-move1.C | 87 +++++++++++++++++++++++++ 4 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 44e1a60ce24..a098ae1830d 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1229,6 +1229,10 @@ Wselector ObjC ObjC++ Var(warn_selector) Warning Warn if a selector has multiple methods. +Wself-move +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn when a value is moved to itself with std::move. + Wsequence-point C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about possible violations of sequence point rules. diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 6e4f23af982..f05913c0fac 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -8897,7 +8897,51 @@ cp_build_c_cast (location_t loc, tree type, tree expr, return error_mark_node; } -\f + +/* Warn when a value is moved to itself with std::move. LHS is the target, + RHS may be the std::move call, and LOC is the location of the whole + assignment. */ + +static void +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) +{ + if (!warn_self_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + if (processing_template_decl) + return; + + /* We're looking for *std::move<T&> ((T &) &arg), or + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ + if (!REFERENCE_REF_P (rhs) + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) + return; + tree fn = TREE_OPERAND (rhs, 0); + if (!is_std_move_p (fn)) + return; + tree arg = CALL_EXPR_ARG (fn, 0); + if (TREE_CODE (arg) != NOP_EXPR) + return; + /* Strip the (T &). */ + arg = TREE_OPERAND (arg, 0); + /* Strip the (T *) or &. */ + arg = TREE_OPERAND (arg, 0); + arg = convert_from_reference (arg); + /* So that we catch (i) = std::move (i);. */ + lhs = maybe_undo_parenthesized_ref (lhs); + STRIP_ANY_LOCATION_WRAPPER (lhs); + if (cp_tree_equal (lhs, arg)) + { + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) + inform (loc, "remove %<std::move%> call"); + } +} + /* For use from the C common bits. */ tree build_modify_expr (location_t location, @@ -9101,6 +9145,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, if (modifycode == NOP_EXPR) { + maybe_warn_self_move (loc, lhs, rhs); + if (c_dialect_objc ()) { result = objc_maybe_build_modify_expr (lhs, rhs); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index f3e9429b2ca..28cf36b94c6 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ in the following sections. -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol @@ -5841,6 +5841,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol +-Wself-move @r{(only for C++)} @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol -Wsizeof-array-div @gol @@ -6826,6 +6827,26 @@ of a declaration: This warning is enabled by @option{-Wall}. +@item -Wno-self-move @r{(C++ and Objective-C++ only)} +@opindex Wself-move +@opindex Wno-self-move +This warning warns when a value is moved to itself with @code{std::move}. +Such a @code{std::move} has no effect. + +@smallexample +struct T @{ +@dots{} +@}; +void fn() +@{ + T t; + @dots{} + t = std::move (t); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsequence-point @opindex Wsequence-point @opindex Wno-sequence-point diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C new file mode 100644 index 00000000000..4a6dbad4533 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C @@ -0,0 +1,87 @@ +// PR c++/81159 +// { dg-do compile { target c++11 } } +// { dg-options "-Wself-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); } +} + +int g; + +struct S { + int x; + S(S&& o) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + x = std::move (o.x); + o.x = std::move (x); + o.x = std::move (o.x); // { dg-warning "moving a variable to itself" } + } + void foo (int x) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + } +}; + +struct X { + int x; + X(int x) : x(std::move (x)) { } +}; + +struct A {}; +struct B { A a; }; +struct C { C(); ~C(); }; +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; + +void +test () +{ + int i = 42; + i = std::move (i); // { dg-warning "moving a variable to itself" } + (i) = std::move (i); // { dg-warning "moving a variable to itself" } + + g = std::move (g); // { dg-warning "moving a variable to itself" } + (g) = std::move (g); // { dg-warning "moving a variable to itself" } + + A a; + a = std::move (a); // { dg-warning "moving a variable to itself" } + + B b; + b = std::move (b); // { dg-warning "moving a variable to itself" } + b.a = std::move (b.a); // { dg-warning "moving a variable to itself" } + + C c; + c = std::move (c); // { dg-warning "moving a variable to itself" } + D d; + d = std::move (d); // { dg-warning "moving a variable to itself" } +} + +template<typename T> +void ttest () +{ + T t; + t = std::move (t); // { dg-warning "moving a variable to itself" } +} + +template void ttest<A>(); + +void +testref (int &r, int &&rr) +{ + r = std::move (r); // { dg-warning "moving a variable to itself" } + rr = std::move (rr); // { dg-warning "moving a variable to itself" } +} base-commit: 9385cd9c74cf6662f43038aafe4d2467899f322e -- 2.37.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] c++: Implement -Wself-move warning [PR81159] 2022-08-09 16:37 [PATCH] c++: Implement -Wself-move warning [PR81159] Marek Polacek @ 2022-08-15 19:54 ` Jason Merrill 2022-08-18 20:19 ` [PATCH v2] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-08-15 19:54 UTC (permalink / raw) To: Marek Polacek, GCC Patches On 8/9/22 09:37, Marek Polacek wrote: > About 5 years ago we got a request to implement -Wself-move, which > warns about useless moves like this: > > int x; > x = std::move (x); > > This patch implements that warning. > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > PR c++/81159 > > gcc/c-family/ChangeLog: > > * c.opt (Wself-move): New option. > > gcc/cp/ChangeLog: > > * typeck.cc (maybe_warn_self_move): New. > (cp_build_modify_expr): Call maybe_warn_self_move. > > gcc/ChangeLog: > > * doc/invoke.texi: Document -Wself-move. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wself-move1.C: New test. > --- > gcc/c-family/c.opt | 4 ++ > gcc/cp/typeck.cc | 48 +++++++++++++- > gcc/doc/invoke.texi | 23 ++++++- > gcc/testsuite/g++.dg/warn/Wself-move1.C | 87 +++++++++++++++++++++++++ > 4 files changed, 160 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index 44e1a60ce24..a098ae1830d 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -1229,6 +1229,10 @@ Wselector > ObjC ObjC++ Var(warn_selector) Warning > Warn if a selector has multiple methods. > > +Wself-move > +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) > +Warn when a value is moved to itself with std::move. > + > Wsequence-point > C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) > Warn about possible violations of sequence point rules. > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 6e4f23af982..f05913c0fac 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -8897,7 +8897,51 @@ cp_build_c_cast (location_t loc, tree type, tree expr, > > return error_mark_node; > } > -\f > + > +/* Warn when a value is moved to itself with std::move. LHS is the target, > + RHS may be the std::move call, and LOC is the location of the whole > + assignment. */ > + > +static void > +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > +{ > + if (!warn_self_move) > + return; > + > + /* C++98 doesn't know move. */ > + if (cxx_dialect < cxx11) > + return; > + > + if (processing_template_decl) > + return; > + > + /* We're looking for *std::move<T&> ((T &) &arg), or > + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ > + if (!REFERENCE_REF_P (rhs) > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > + return; > + tree fn = TREE_OPERAND (rhs, 0); > + if (!is_std_move_p (fn)) > + return; > + tree arg = CALL_EXPR_ARG (fn, 0); > + if (TREE_CODE (arg) != NOP_EXPR) > + return; > + /* Strip the (T &). */ > + arg = TREE_OPERAND (arg, 0); > + /* Strip the (T *) or &. */ > + arg = TREE_OPERAND (arg, 0); Are you sure these are the only two expressions that can make it here? What if the argument to move is *Tptr? > + arg = convert_from_reference (arg); > + /* So that we catch (i) = std::move (i);. */ > + lhs = maybe_undo_parenthesized_ref (lhs); > + STRIP_ANY_LOCATION_WRAPPER (lhs); > + if (cp_tree_equal (lhs, arg)) > + { > + auto_diagnostic_group d; > + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) > + inform (loc, "remove %<std::move%> call"); > + } > +} > + > /* For use from the C common bits. */ > tree > build_modify_expr (location_t location, > @@ -9101,6 +9145,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, > > if (modifycode == NOP_EXPR) > { > + maybe_warn_self_move (loc, lhs, rhs); > + > if (c_dialect_objc ()) > { > result = objc_maybe_build_modify_expr (lhs, rhs); > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index f3e9429b2ca..28cf36b94c6 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -264,7 +264,7 @@ in the following sections. > -Wreorder -Wregister @gol > -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol > -Wno-non-template-friend -Wold-style-cast @gol > --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol > -Wsized-deallocation -Wsuggest-final-methods @gol > -Wsuggest-final-types -Wsuggest-override @gol > -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > @@ -5841,6 +5841,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. > -Wreorder @gol > -Wrestrict @gol > -Wreturn-type @gol > +-Wself-move @r{(only for C++)} @gol > -Wsequence-point @gol > -Wsign-compare @r{(only in C++)} @gol > -Wsizeof-array-div @gol > @@ -6826,6 +6827,26 @@ of a declaration: > > This warning is enabled by @option{-Wall}. > > +@item -Wno-self-move @r{(C++ and Objective-C++ only)} > +@opindex Wself-move > +@opindex Wno-self-move > +This warning warns when a value is moved to itself with @code{std::move}. > +Such a @code{std::move} has no effect. ...unless it naively breaks the object, like T(T&& ot): data(ot.data) { ot.data = nullptr; } // oops > +@smallexample > +struct T @{ > +@dots{} > +@}; > +void fn() > +@{ > + T t; > + @dots{} > + t = std::move (t); > +@} > +@end smallexample > + > +This warning is enabled by @option{-Wall}. > + > @item -Wsequence-point > @opindex Wsequence-point > @opindex Wno-sequence-point > diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C > new file mode 100644 > index 00000000000..4a6dbad4533 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C > @@ -0,0 +1,87 @@ > +// PR c++/81159 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wself-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); } > +} > + > +int g; > + > +struct S { > + int x; > + S(S&& o) { > + x = std::move (x); // { dg-warning "moving a variable to itself" } > + x = std::move (o.x); > + o.x = std::move (x); > + o.x = std::move (o.x); // { dg-warning "moving a variable to itself" } > + } > + void foo (int x) { > + x = std::move (x); // { dg-warning "moving a variable to itself" } > + } > +}; > + > +struct X { > + int x; > + X(int x) : x(std::move (x)) { } > +}; > + > +struct A {}; > +struct B { A a; }; > +struct C { C(); ~C(); }; > +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; > + > +void > +test () > +{ > + int i = 42; > + i = std::move (i); // { dg-warning "moving a variable to itself" } > + (i) = std::move (i); // { dg-warning "moving a variable to itself" } > + > + g = std::move (g); // { dg-warning "moving a variable to itself" } > + (g) = std::move (g); // { dg-warning "moving a variable to itself" } > + > + A a; > + a = std::move (a); // { dg-warning "moving a variable to itself" } > + > + B b; > + b = std::move (b); // { dg-warning "moving a variable to itself" } > + b.a = std::move (b.a); // { dg-warning "moving a variable to itself" } > + > + C c; > + c = std::move (c); // { dg-warning "moving a variable to itself" } > + D d; > + d = std::move (d); // { dg-warning "moving a variable to itself" } > +} > + > +template<typename T> > +void ttest () > +{ > + T t; > + t = std::move (t); // { dg-warning "moving a variable to itself" } > +} > + > +template void ttest<A>(); > + > +void > +testref (int &r, int &&rr) > +{ > + r = std::move (r); // { dg-warning "moving a variable to itself" } > + rr = std::move (rr); // { dg-warning "moving a variable to itself" } > +} > > base-commit: 9385cd9c74cf6662f43038aafe4d2467899f322e ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2] c++: Implement -Wself-move warning [PR81159] 2022-08-15 19:54 ` Jason Merrill @ 2022-08-18 20:19 ` Marek Polacek 2022-08-19 0:33 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-08-18 20:19 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches On Mon, Aug 15, 2022 at 03:54:05PM -0400, Jason Merrill wrote: > On 8/9/22 09:37, Marek Polacek wrote: > > + /* We're looking for *std::move<T&> ((T &) &arg), or > > + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ > > + if (!REFERENCE_REF_P (rhs) > > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > > + return; > > + tree fn = TREE_OPERAND (rhs, 0); > > + if (!is_std_move_p (fn)) > > + return; > > + tree arg = CALL_EXPR_ARG (fn, 0); > > + if (TREE_CODE (arg) != NOP_EXPR) > > + return; > > + /* Strip the (T &). */ > > + arg = TREE_OPERAND (arg, 0); > > + /* Strip the (T *) or &. */ > > + arg = TREE_OPERAND (arg, 0); > > Are you sure these are the only two expressions that can make it here? What > if the argument to move is *Tptr? Not 100% sure but I couldn't find any other form. For *Tptr we get *std::move<int*&> ((int * &) &Tptr) so it works as expected as well. I've extended the existing test to test this too. > > @@ -6826,6 +6827,26 @@ of a declaration: > > This warning is enabled by @option{-Wall}. > > +@item -Wno-self-move @r{(C++ and Objective-C++ only)} > > +@opindex Wself-move > > +@opindex Wno-self-move > > +This warning warns when a value is moved to itself with @code{std::move}. > > +Such a @code{std::move} has no effect. > > ...unless it naively breaks the object, like > > T(T&& ot): data(ot.data) { ot.data = nullptr; } // oops "If you try to move me I'll disappear!" I've added the weasel word: "typically has no effect." Or do we want to say more? -- >8 -- About 5 years ago we got a request to implement -Wself-move, which warns about useless moves like this: int x; x = std::move (x); This patch implements that warning. PR c++/81159 gcc/c-family/ChangeLog: * c.opt (Wself-move): New option. gcc/cp/ChangeLog: * typeck.cc (maybe_warn_self_move): New. (cp_build_modify_expr): Call maybe_warn_self_move. gcc/ChangeLog: * doc/invoke.texi: Document -Wself-move. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move1.C: New test. --- gcc/c-family/c.opt | 4 + gcc/cp/typeck.cc | 48 ++++++++++- gcc/doc/invoke.texi | 23 +++++- gcc/testsuite/g++.dg/warn/Wself-move1.C | 105 ++++++++++++++++++++++++ 4 files changed, 178 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index dfdebd596ef..f776efd39d8 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1229,6 +1229,10 @@ Wselector ObjC ObjC++ Var(warn_selector) Warning Warn if a selector has multiple methods. +Wself-move +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn when a value is moved to itself with std::move. + Wsequence-point C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about possible violations of sequence point rules. diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 992ebfd99fb..cbc32a7c8ca 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -8897,7 +8897,51 @@ cp_build_c_cast (location_t loc, tree type, tree expr, return error_mark_node; } -\f + +/* Warn when a value is moved to itself with std::move. LHS is the target, + RHS may be the std::move call, and LOC is the location of the whole + assignment. */ + +static void +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) +{ + if (!warn_self_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + if (processing_template_decl) + return; + + /* We're looking for *std::move<T&> ((T &) &arg), or + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ + if (!REFERENCE_REF_P (rhs) + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) + return; + tree fn = TREE_OPERAND (rhs, 0); + if (!is_std_move_p (fn)) + return; + tree arg = CALL_EXPR_ARG (fn, 0); + if (TREE_CODE (arg) != NOP_EXPR) + return; + /* Strip the (T &). */ + arg = TREE_OPERAND (arg, 0); + /* Strip the (T *) or &. */ + arg = TREE_OPERAND (arg, 0); + arg = convert_from_reference (arg); + /* So that we catch (i) = std::move (i);. */ + lhs = maybe_undo_parenthesized_ref (lhs); + STRIP_ANY_LOCATION_WRAPPER (lhs); + if (cp_tree_equal (lhs, arg)) + { + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) + inform (loc, "remove %<std::move%> call"); + } +} + /* For use from the C common bits. */ tree build_modify_expr (location_t location, @@ -9101,6 +9145,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, if (modifycode == NOP_EXPR) { + maybe_warn_self_move (loc, lhs, rhs); + if (c_dialect_objc ()) { result = objc_maybe_build_modify_expr (lhs, rhs); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index f65d351a5fc..5dea3fee124 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ in the following sections. -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol @@ -5843,6 +5843,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol +-Wself-move @r{(only for C++)} @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol -Wsizeof-array-div @gol @@ -6828,6 +6829,26 @@ of a declaration: This warning is enabled by @option{-Wall}. +@item -Wno-self-move @r{(C++ and Objective-C++ only)} +@opindex Wself-move +@opindex Wno-self-move +This warning warns when a value is moved to itself with @code{std::move}. +Such a @code{std::move} typically has no effect. + +@smallexample +struct T @{ +@dots{} +@}; +void fn() +@{ + T t; + @dots{} + t = std::move (t); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsequence-point @opindex Wsequence-point @opindex Wno-sequence-point diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C new file mode 100644 index 00000000000..b12f0d43dad --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C @@ -0,0 +1,105 @@ +// PR c++/81159 +// { dg-do compile { target c++11 } } +// { dg-options "-Wself-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); } +} + +int g; + +struct S { + int x; + S(S&& o) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + x = std::move (o.x); + o.x = std::move (x); + o.x = std::move (o.x); // { dg-warning "moving a variable to itself" } + } + void foo (int x) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + } +}; + +struct X { + int x; + X(int x) : x(std::move (x)) { } +}; + +struct A {}; +struct B { A a; }; +struct C { C(); ~C(); }; +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; + +void +test () +{ + int i = 42; + i = std::move (i); // { dg-warning "moving a variable to itself" } + (i) = std::move (i); // { dg-warning "moving a variable to itself" } + + g = std::move (g); // { dg-warning "moving a variable to itself" } + (g) = std::move (g); // { dg-warning "moving a variable to itself" } + + A a; + a = std::move (a); // { dg-warning "moving a variable to itself" } + + B b; + b = std::move (b); // { dg-warning "moving a variable to itself" } + b.a = std::move (b.a); // { dg-warning "moving a variable to itself" } + + C c; + c = std::move (c); // { dg-warning "moving a variable to itself" } + D d; + d = std::move (d); // { dg-warning "moving a variable to itself" } +} + +template<typename T> +void ttest () +{ + T t; + t = std::move (t); // { dg-warning "moving a variable to itself" } +} + +template void ttest<A>(); + +void +testref (int &r, int &&rr) +{ + r = std::move (r); // { dg-warning "moving a variable to itself" } + rr = std::move (rr); // { dg-warning "moving a variable to itself" } +} + +// Test various other arguments to std::move. +template<typename T> +void +testargs (T *Tptr, T& Tref, T&& Trref, const T *Tcptr) +{ + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } + Tref = std::move (Tref); // { dg-warning "moving a variable to itself" } + Trref = std::move (Trref); // { dg-warning "moving a variable to itself" } + Tcptr = std::move (Tcptr); // { dg-warning "moving a variable to itself" } +} + +void +call_testargs () +{ + int i = 42; + testargs<int>(&i, i, 42, &i); +} base-commit: ca170ed9f8a086ca7e1eec841882b6bed9ec1a3a -- 2.37.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2] c++: Implement -Wself-move warning [PR81159] 2022-08-18 20:19 ` [PATCH v2] " Marek Polacek @ 2022-08-19 0:33 ` Jason Merrill 2022-08-19 22:34 ` [PATCH v3] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-08-19 0:33 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches On 8/18/22 13:19, Marek Polacek wrote: > On Mon, Aug 15, 2022 at 03:54:05PM -0400, Jason Merrill wrote: >> On 8/9/22 09:37, Marek Polacek wrote: >>> + /* We're looking for *std::move<T&> ((T &) &arg), or >>> + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ >>> + if (!REFERENCE_REF_P (rhs) >>> + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) >>> + return; >>> + tree fn = TREE_OPERAND (rhs, 0); >>> + if (!is_std_move_p (fn)) >>> + return; >>> + tree arg = CALL_EXPR_ARG (fn, 0); >>> + if (TREE_CODE (arg) != NOP_EXPR) >>> + return; >>> + /* Strip the (T &). */ >>> + arg = TREE_OPERAND (arg, 0); >>> + /* Strip the (T *) or &. */ >>> + arg = TREE_OPERAND (arg, 0); >> >> Are you sure these are the only two expressions that can make it here? What >> if the argument to move is *Tptr? > > Not 100% sure but I couldn't find any other form. For *Tptr we get > *std::move<int*&> ((int * &) &Tptr) That likes like what you'd get when the argument is Tptr, not when it's *Tptr. And indeed that's what I see in the testcase: > + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } is missing the * >>> @@ -6826,6 +6827,26 @@ of a declaration: >>> This warning is enabled by @option{-Wall}. >>> +@item -Wno-self-move @r{(C++ and Objective-C++ only)} >>> +@opindex Wself-move >>> +@opindex Wno-self-move >>> +This warning warns when a value is moved to itself with @code{std::move}. >>> +Such a @code{std::move} has no effect. >> >> ...unless it naively breaks the object, like >> >> T(T&& ot): data(ot.data) { ot.data = nullptr; } // oops > > "If you try to move me I'll disappear!" > > I've added the weasel word: "typically has no effect." Or do we want to say > more? > > -- >8 -- > About 5 years ago we got a request to implement -Wself-move, which > warns about useless moves like this: > > int x; > x = std::move (x); > > This patch implements that warning. > > PR c++/81159 > > gcc/c-family/ChangeLog: > > * c.opt (Wself-move): New option. > > gcc/cp/ChangeLog: > > * typeck.cc (maybe_warn_self_move): New. > (cp_build_modify_expr): Call maybe_warn_self_move. > > gcc/ChangeLog: > > * doc/invoke.texi: Document -Wself-move. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wself-move1.C: New test. > --- > gcc/c-family/c.opt | 4 + > gcc/cp/typeck.cc | 48 ++++++++++- > gcc/doc/invoke.texi | 23 +++++- > gcc/testsuite/g++.dg/warn/Wself-move1.C | 105 ++++++++++++++++++++++++ > 4 files changed, 178 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index dfdebd596ef..f776efd39d8 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -1229,6 +1229,10 @@ Wselector > ObjC ObjC++ Var(warn_selector) Warning > Warn if a selector has multiple methods. > > +Wself-move > +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) > +Warn when a value is moved to itself with std::move. > + > Wsequence-point > C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) > Warn about possible violations of sequence point rules. > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 992ebfd99fb..cbc32a7c8ca 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -8897,7 +8897,51 @@ cp_build_c_cast (location_t loc, tree type, tree expr, > > return error_mark_node; > } > -\f > + > +/* Warn when a value is moved to itself with std::move. LHS is the target, > + RHS may be the std::move call, and LOC is the location of the whole > + assignment. */ > + > +static void > +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > +{ > + if (!warn_self_move) > + return; > + > + /* C++98 doesn't know move. */ > + if (cxx_dialect < cxx11) > + return; > + > + if (processing_template_decl) > + return; > + > + /* We're looking for *std::move<T&> ((T &) &arg), or > + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ > + if (!REFERENCE_REF_P (rhs) > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > + return; > + tree fn = TREE_OPERAND (rhs, 0); > + if (!is_std_move_p (fn)) > + return; > + tree arg = CALL_EXPR_ARG (fn, 0); > + if (TREE_CODE (arg) != NOP_EXPR) > + return; > + /* Strip the (T &). */ > + arg = TREE_OPERAND (arg, 0); > + /* Strip the (T *) or &. */ > + arg = TREE_OPERAND (arg, 0); > + arg = convert_from_reference (arg); > + /* So that we catch (i) = std::move (i);. */ > + lhs = maybe_undo_parenthesized_ref (lhs); > + STRIP_ANY_LOCATION_WRAPPER (lhs); > + if (cp_tree_equal (lhs, arg)) > + { > + auto_diagnostic_group d; > + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) > + inform (loc, "remove %<std::move%> call"); > + } > +} > + > /* For use from the C common bits. */ > tree > build_modify_expr (location_t location, > @@ -9101,6 +9145,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, > > if (modifycode == NOP_EXPR) > { > + maybe_warn_self_move (loc, lhs, rhs); > + > if (c_dialect_objc ()) > { > result = objc_maybe_build_modify_expr (lhs, rhs); > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index f65d351a5fc..5dea3fee124 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -264,7 +264,7 @@ in the following sections. > -Wreorder -Wregister @gol > -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol > -Wno-non-template-friend -Wold-style-cast @gol > --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol > -Wsized-deallocation -Wsuggest-final-methods @gol > -Wsuggest-final-types -Wsuggest-override @gol > -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > @@ -5843,6 +5843,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. > -Wreorder @gol > -Wrestrict @gol > -Wreturn-type @gol > +-Wself-move @r{(only for C++)} @gol > -Wsequence-point @gol > -Wsign-compare @r{(only in C++)} @gol > -Wsizeof-array-div @gol > @@ -6828,6 +6829,26 @@ of a declaration: > > This warning is enabled by @option{-Wall}. > > +@item -Wno-self-move @r{(C++ and Objective-C++ only)} > +@opindex Wself-move > +@opindex Wno-self-move > +This warning warns when a value is moved to itself with @code{std::move}. > +Such a @code{std::move} typically has no effect. > + > +@smallexample > +struct T @{ > +@dots{} > +@}; > +void fn() > +@{ > + T t; > + @dots{} > + t = std::move (t); > +@} > +@end smallexample > + > +This warning is enabled by @option{-Wall}. > + > @item -Wsequence-point > @opindex Wsequence-point > @opindex Wno-sequence-point > diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C > new file mode 100644 > index 00000000000..b12f0d43dad > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C > @@ -0,0 +1,105 @@ > +// PR c++/81159 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wself-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); } > +} > + > +int g; > + > +struct S { > + int x; > + S(S&& o) { > + x = std::move (x); // { dg-warning "moving a variable to itself" } > + x = std::move (o.x); > + o.x = std::move (x); > + o.x = std::move (o.x); // { dg-warning "moving a variable to itself" } > + } > + void foo (int x) { > + x = std::move (x); // { dg-warning "moving a variable to itself" } > + } > +}; > + > +struct X { > + int x; > + X(int x) : x(std::move (x)) { } > +}; > + > +struct A {}; > +struct B { A a; }; > +struct C { C(); ~C(); }; > +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; > + > +void > +test () > +{ > + int i = 42; > + i = std::move (i); // { dg-warning "moving a variable to itself" } > + (i) = std::move (i); // { dg-warning "moving a variable to itself" } > + > + g = std::move (g); // { dg-warning "moving a variable to itself" } > + (g) = std::move (g); // { dg-warning "moving a variable to itself" } > + > + A a; > + a = std::move (a); // { dg-warning "moving a variable to itself" } > + > + B b; > + b = std::move (b); // { dg-warning "moving a variable to itself" } > + b.a = std::move (b.a); // { dg-warning "moving a variable to itself" } > + > + C c; > + c = std::move (c); // { dg-warning "moving a variable to itself" } > + D d; > + d = std::move (d); // { dg-warning "moving a variable to itself" } > +} > + > +template<typename T> > +void ttest () > +{ > + T t; > + t = std::move (t); // { dg-warning "moving a variable to itself" } > +} > + > +template void ttest<A>(); > + > +void > +testref (int &r, int &&rr) > +{ > + r = std::move (r); // { dg-warning "moving a variable to itself" } > + rr = std::move (rr); // { dg-warning "moving a variable to itself" } > +} > + > +// Test various other arguments to std::move. > +template<typename T> > +void > +testargs (T *Tptr, T& Tref, T&& Trref, const T *Tcptr) > +{ > + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } > + Tref = std::move (Tref); // { dg-warning "moving a variable to itself" } > + Trref = std::move (Trref); // { dg-warning "moving a variable to itself" } > + Tcptr = std::move (Tcptr); // { dg-warning "moving a variable to itself" } > +} > + > +void > +call_testargs () > +{ > + int i = 42; > + testargs<int>(&i, i, 42, &i); > +} > > base-commit: ca170ed9f8a086ca7e1eec841882b6bed9ec1a3a ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v3] c++: Implement -Wself-move warning [PR81159] 2022-08-19 0:33 ` Jason Merrill @ 2022-08-19 22:34 ` Marek Polacek 2022-08-20 21:31 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-08-19 22:34 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches On Thu, Aug 18, 2022 at 08:33:47PM -0400, Jason Merrill wrote: > On 8/18/22 13:19, Marek Polacek wrote: > > On Mon, Aug 15, 2022 at 03:54:05PM -0400, Jason Merrill wrote: > > > On 8/9/22 09:37, Marek Polacek wrote: > > > > + /* We're looking for *std::move<T&> ((T &) &arg), or > > > > + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ > > > > + if (!REFERENCE_REF_P (rhs) > > > > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > > > > + return; > > > > + tree fn = TREE_OPERAND (rhs, 0); > > > > + if (!is_std_move_p (fn)) > > > > + return; > > > > + tree arg = CALL_EXPR_ARG (fn, 0); > > > > + if (TREE_CODE (arg) != NOP_EXPR) > > > > + return; > > > > + /* Strip the (T &). */ > > > > + arg = TREE_OPERAND (arg, 0); > > > > + /* Strip the (T *) or &. */ > > > > + arg = TREE_OPERAND (arg, 0); > > > > > > Are you sure these are the only two expressions that can make it here? What > > > if the argument to move is *Tptr? > > > > Not 100% sure but I couldn't find any other form. For *Tptr we get > > *std::move<int*&> ((int * &) &Tptr) > > That likes like what you'd get when the argument is Tptr, not when it's > *Tptr. And indeed that's what I see in the testcase: > > > + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } > > is missing the * Duh, sorry. The previous patch didn't handle the *Tptr case. Further poking revealed that we need special care to handle (*(Tptr)) and **Tptr etc. So in this patch I'm stripping all *s and V_C_Es. Sigh. Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- About 5 years ago we got a request to implement -Wself-move, which warns about useless moves like this: int x; x = std::move (x); This patch implements that warning. PR c++/81159 gcc/c-family/ChangeLog: * c.opt (Wself-move): New option. gcc/cp/ChangeLog: * typeck.cc (maybe_warn_self_move): New. (cp_build_modify_expr): Call maybe_warn_self_move. gcc/ChangeLog: * doc/invoke.texi: Document -Wself-move. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move1.C: New test. --- gcc/c-family/c.opt | 4 + gcc/cp/typeck.cc | 51 +++++++++- gcc/doc/invoke.texi | 23 ++++- gcc/testsuite/g++.dg/warn/Wself-move1.C | 122 ++++++++++++++++++++++++ 4 files changed, 198 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index dfdebd596ef..f776efd39d8 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1229,6 +1229,10 @@ Wselector ObjC ObjC++ Var(warn_selector) Warning Warn if a selector has multiple methods. +Wself-move +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn when a value is moved to itself with std::move. + Wsequence-point C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about possible violations of sequence point rules. diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 992ebfd99fb..b2bd13db1b6 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -8897,7 +8897,54 @@ cp_build_c_cast (location_t loc, tree type, tree expr, return error_mark_node; } -\f + +/* Warn when a value is moved to itself with std::move. LHS is the target, + RHS may be the std::move call, and LOC is the location of the whole + assignment. */ + +static void +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) +{ + if (!warn_self_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + if (processing_template_decl) + return; + + if (!REFERENCE_REF_P (rhs) + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) + return; + tree fn = TREE_OPERAND (rhs, 0); + if (!is_std_move_p (fn)) + return; + + /* Just a little helper to strip * and various NOPs. */ + auto extract_op = [] (tree &op) { + STRIP_NOPS (op); + while (INDIRECT_REF_P (op)) + op = TREE_OPERAND (op, 0); + op = maybe_undo_parenthesized_ref (op); + STRIP_ANY_LOCATION_WRAPPER (op); + }; + + tree arg = CALL_EXPR_ARG (fn, 0); + extract_op (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + extract_op (lhs); + + if (cp_tree_equal (lhs, arg)) + { + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) + inform (loc, "remove %<std::move%> call"); + } +} + /* For use from the C common bits. */ tree build_modify_expr (location_t location, @@ -9101,6 +9148,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, if (modifycode == NOP_EXPR) { + maybe_warn_self_move (loc, lhs, rhs); + if (c_dialect_objc ()) { result = objc_maybe_build_modify_expr (lhs, rhs); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index f65d351a5fc..5dea3fee124 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ in the following sections. -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol @@ -5843,6 +5843,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol +-Wself-move @r{(only for C++)} @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol -Wsizeof-array-div @gol @@ -6828,6 +6829,26 @@ of a declaration: This warning is enabled by @option{-Wall}. +@item -Wno-self-move @r{(C++ and Objective-C++ only)} +@opindex Wself-move +@opindex Wno-self-move +This warning warns when a value is moved to itself with @code{std::move}. +Such a @code{std::move} typically has no effect. + +@smallexample +struct T @{ +@dots{} +@}; +void fn() +@{ + T t; + @dots{} + t = std::move (t); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsequence-point @opindex Wsequence-point @opindex Wno-sequence-point diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C new file mode 100644 index 00000000000..601d8c1521a --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C @@ -0,0 +1,122 @@ +// PR c++/81159 +// { dg-do compile { target c++11 } } +// { dg-options "-Wself-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); } +} + +int g; + +struct S { + int x; + S(S&& o) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + x = std::move (o.x); + o.x = std::move (x); + o.x = std::move (o.x); // { dg-warning "moving a variable to itself" } + } + void foo (int x) { + x = std::move (x); // { dg-warning "moving a variable to itself" } + } +}; + +struct X { + int x; + X(int x) : x(std::move (x)) { } +}; + +struct A {}; +struct B { A a; }; +struct C { C(); ~C(); }; +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; + +void +test () +{ + int i = 42; + i = std::move (i); // { dg-warning "moving a variable to itself" } + (i) = std::move (i); // { dg-warning "moving a variable to itself" } + + g = std::move (g); // { dg-warning "moving a variable to itself" } + (g) = std::move (g); // { dg-warning "moving a variable to itself" } + + A a; + a = std::move (a); // { dg-warning "moving a variable to itself" } + + B b; + b = std::move (b); // { dg-warning "moving a variable to itself" } + b.a = std::move (b.a); // { dg-warning "moving a variable to itself" } + + C c; + c = std::move (c); // { dg-warning "moving a variable to itself" } + D d; + d = std::move (d); // { dg-warning "moving a variable to itself" } +} + +template<typename T> +void ttest () +{ + T t; + t = std::move (t); // { dg-warning "moving a variable to itself" } +} + +template void ttest<A>(); + +void +testref (int &r, int &&rr) +{ + r = std::move (r); // { dg-warning "moving a variable to itself" } + rr = std::move (rr); // { dg-warning "moving a variable to itself" } +} + +// Test various other arguments to std::move. +template<typename T> +void +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) +{ + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } + *Tptr = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + *Tptr = std::move (*(Tptr)); // { dg-warning "moving a variable to itself" } + *(Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + *(Tptr + 1) = std::move (*Tptr + 1); + *(Tptr + 1) = std::move (*Tptr + 2); + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + *Tpptr = std::move (*Tpptr); // { dg-warning "moving a variable to itself" } + **Tpptr = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } + Tref = std::move (Tref); // { dg-warning "moving a variable to itself" } + Trref = std::move (Trref); // { dg-warning "moving a variable to itself" } + Tcptr = std::move (Tcptr); // { dg-warning "moving a variable to itself" } + (Tptr) = std::move (Tptr); // { dg-warning "moving a variable to itself" } + (*Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving a variable to itself" } + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } + (Tref) = std::move (Tref); // { dg-warning "moving a variable to itself" } + (Trref) = std::move (Trref); // { dg-warning "moving a variable to itself" } + (Tcptr) = std::move (Tcptr); // { dg-warning "moving a variable to itself" } +} + +void +call_testargs () +{ + int i = 42; + int *p = &i; + testargs<int>(&i, &p, i, 42, &i); +} base-commit: 713ec97e593bd4d9915a13bc4047f064fec0e24a -- 2.37.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v3] c++: Implement -Wself-move warning [PR81159] 2022-08-19 22:34 ` [PATCH v3] " Marek Polacek @ 2022-08-20 21:31 ` Jason Merrill 2022-08-23 16:39 ` [PATCH v4] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-08-20 21:31 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches On 8/19/22 15:34, Marek Polacek wrote: > On Thu, Aug 18, 2022 at 08:33:47PM -0400, Jason Merrill wrote: >> On 8/18/22 13:19, Marek Polacek wrote: >>> On Mon, Aug 15, 2022 at 03:54:05PM -0400, Jason Merrill wrote: >>>> On 8/9/22 09:37, Marek Polacek wrote: >>>>> + /* We're looking for *std::move<T&> ((T &) &arg), or >>>>> + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ >>>>> + if (!REFERENCE_REF_P (rhs) >>>>> + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) >>>>> + return; >>>>> + tree fn = TREE_OPERAND (rhs, 0); >>>>> + if (!is_std_move_p (fn)) >>>>> + return; >>>>> + tree arg = CALL_EXPR_ARG (fn, 0); >>>>> + if (TREE_CODE (arg) != NOP_EXPR) >>>>> + return; >>>>> + /* Strip the (T &). */ >>>>> + arg = TREE_OPERAND (arg, 0); >>>>> + /* Strip the (T *) or &. */ >>>>> + arg = TREE_OPERAND (arg, 0); >>>> >>>> Are you sure these are the only two expressions that can make it here? What >>>> if the argument to move is *Tptr? >>> >>> Not 100% sure but I couldn't find any other form. For *Tptr we get >>> *std::move<int*&> ((int * &) &Tptr) >> >> That likes like what you'd get when the argument is Tptr, not when it's >> *Tptr. And indeed that's what I see in the testcase: >> >>> + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } >> >> is missing the * > > Duh, sorry. The previous patch didn't handle the *Tptr case. Further poking > revealed that we need special care to handle (*(Tptr)) and **Tptr etc. So in > this patch I'm stripping all *s and V_C_Es. Sigh. Ah, I was just thinking that the old patch needed more checking of TREE_CODEs. But I suppose it's also good to diagnose these cases as well. > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > About 5 years ago we got a request to implement -Wself-move, which > warns about useless moves like this: > > int x; > x = std::move (x); > > This patch implements that warning. > > PR c++/81159 > > gcc/c-family/ChangeLog: > > * c.opt (Wself-move): New option. > > gcc/cp/ChangeLog: > > * typeck.cc (maybe_warn_self_move): New. > (cp_build_modify_expr): Call maybe_warn_self_move. > > gcc/ChangeLog: > > * doc/invoke.texi: Document -Wself-move. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wself-move1.C: New test. > --- > gcc/c-family/c.opt | 4 + > gcc/cp/typeck.cc | 51 +++++++++- > gcc/doc/invoke.texi | 23 ++++- > gcc/testsuite/g++.dg/warn/Wself-move1.C | 122 ++++++++++++++++++++++++ > 4 files changed, 198 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index dfdebd596ef..f776efd39d8 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -1229,6 +1229,10 @@ Wselector > ObjC ObjC++ Var(warn_selector) Warning > Warn if a selector has multiple methods. > > +Wself-move > +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) > +Warn when a value is moved to itself with std::move. > + > Wsequence-point > C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) > Warn about possible violations of sequence point rules. > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 992ebfd99fb..b2bd13db1b6 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -8897,7 +8897,54 @@ cp_build_c_cast (location_t loc, tree type, tree expr, > > return error_mark_node; > } > -\f > + > +/* Warn when a value is moved to itself with std::move. LHS is the target, > + RHS may be the std::move call, and LOC is the location of the whole > + assignment. */ > + > +static void > +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > +{ > + if (!warn_self_move) > + return; > + > + /* C++98 doesn't know move. */ > + if (cxx_dialect < cxx11) > + return; > + > + if (processing_template_decl) > + return; > + > + if (!REFERENCE_REF_P (rhs) > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > + return; > + tree fn = TREE_OPERAND (rhs, 0); > + if (!is_std_move_p (fn)) > + return; > + > + /* Just a little helper to strip * and various NOPs. */ > + auto extract_op = [] (tree &op) { > + STRIP_NOPS (op); > + while (INDIRECT_REF_P (op)) > + op = TREE_OPERAND (op, 0); > + op = maybe_undo_parenthesized_ref (op); > + STRIP_ANY_LOCATION_WRAPPER (op); > + }; > + > + tree arg = CALL_EXPR_ARG (fn, 0); > + extract_op (arg); > + if (TREE_CODE (arg) == ADDR_EXPR) > + arg = TREE_OPERAND (arg, 0); > + extract_op (lhs); > + > + if (cp_tree_equal (lhs, arg)) > + { > + auto_diagnostic_group d; > + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) Maybe only say "variable" when the original operand is in fact a variable? > + inform (loc, "remove %<std::move%> call"); > + } > +} > + > /* For use from the C common bits. */ > tree > build_modify_expr (location_t location, > @@ -9101,6 +9148,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, > > if (modifycode == NOP_EXPR) > { > + maybe_warn_self_move (loc, lhs, rhs); > + > if (c_dialect_objc ()) > { > result = objc_maybe_build_modify_expr (lhs, rhs); > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index f65d351a5fc..5dea3fee124 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -264,7 +264,7 @@ in the following sections. > -Wreorder -Wregister @gol > -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol > -Wno-non-template-friend -Wold-style-cast @gol > --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol > -Wsized-deallocation -Wsuggest-final-methods @gol > -Wsuggest-final-types -Wsuggest-override @gol > -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > @@ -5843,6 +5843,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. > -Wreorder @gol > -Wrestrict @gol > -Wreturn-type @gol > +-Wself-move @r{(only for C++)} @gol > -Wsequence-point @gol > -Wsign-compare @r{(only in C++)} @gol > -Wsizeof-array-div @gol > @@ -6828,6 +6829,26 @@ of a declaration: > > This warning is enabled by @option{-Wall}. > > +@item -Wno-self-move @r{(C++ and Objective-C++ only)} > +@opindex Wself-move > +@opindex Wno-self-move > +This warning warns when a value is moved to itself with @code{std::move}. > +Such a @code{std::move} typically has no effect. > + > +@smallexample > +struct T @{ > +@dots{} > +@}; > +void fn() > +@{ > + T t; > + @dots{} > + t = std::move (t); > +@} > +@end smallexample > + > +This warning is enabled by @option{-Wall}. > + > @item -Wsequence-point > @opindex Wsequence-point > @opindex Wno-sequence-point > diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C > new file mode 100644 > index 00000000000..601d8c1521a > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C > @@ -0,0 +1,122 @@ > +// PR c++/81159 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wself-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); } > +} > + > +int g; > + > +struct S { > + int x; > + S(S&& o) { > + x = std::move (x); // { dg-warning "moving a variable to itself" } > + x = std::move (o.x); > + o.x = std::move (x); > + o.x = std::move (o.x); // { dg-warning "moving a variable to itself" } > + } > + void foo (int x) { > + x = std::move (x); // { dg-warning "moving a variable to itself" } > + } > +}; > + > +struct X { > + int x; > + X(int x) : x(std::move (x)) { } > +}; > + > +struct A {}; > +struct B { A a; }; > +struct C { C(); ~C(); }; > +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; > + > +void > +test () > +{ > + int i = 42; > + i = std::move (i); // { dg-warning "moving a variable to itself" } > + (i) = std::move (i); // { dg-warning "moving a variable to itself" } > + > + g = std::move (g); // { dg-warning "moving a variable to itself" } > + (g) = std::move (g); // { dg-warning "moving a variable to itself" } > + > + A a; > + a = std::move (a); // { dg-warning "moving a variable to itself" } > + > + B b; > + b = std::move (b); // { dg-warning "moving a variable to itself" } > + b.a = std::move (b.a); // { dg-warning "moving a variable to itself" } > + > + C c; > + c = std::move (c); // { dg-warning "moving a variable to itself" } > + D d; > + d = std::move (d); // { dg-warning "moving a variable to itself" } > +} > + > +template<typename T> > +void ttest () > +{ > + T t; > + t = std::move (t); // { dg-warning "moving a variable to itself" } > +} > + > +template void ttest<A>(); > + > +void > +testref (int &r, int &&rr) > +{ > + r = std::move (r); // { dg-warning "moving a variable to itself" } > + rr = std::move (rr); // { dg-warning "moving a variable to itself" } > +} > + > +// Test various other arguments to std::move. > +template<typename T> > +void > +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) > +{ > + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } > + *Tptr = std::move (*Tptr); // { dg-warning "moving a variable to itself" } > + *Tptr = std::move (*(Tptr)); // { dg-warning "moving a variable to itself" } > + *(Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } > + *(Tptr + 1) = std::move (*Tptr + 1); > + *(Tptr + 1) = std::move (*Tptr + 2); Do you mean to have *(Tptr + 1) on both sides? The difference in * operand is surprising. > + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } > + *Tpptr = std::move (*Tpptr); // { dg-warning "moving a variable to itself" } > + **Tpptr = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } > + Tref = std::move (Tref); // { dg-warning "moving a variable to itself" } > + Trref = std::move (Trref); // { dg-warning "moving a variable to itself" } > + Tcptr = std::move (Tcptr); // { dg-warning "moving a variable to itself" } > + (Tptr) = std::move (Tptr); // { dg-warning "moving a variable to itself" } > + (*Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } > + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving a variable to itself" } > + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } > + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving a variable to itself" } > + (Tref) = std::move (Tref); // { dg-warning "moving a variable to itself" } > + (Trref) = std::move (Trref); // { dg-warning "moving a variable to itself" } > + (Tcptr) = std::move (Tcptr); // { dg-warning "moving a variable to itself" } > +} > + > +void > +call_testargs () > +{ > + int i = 42; > + int *p = &i; > + testargs<int>(&i, &p, i, 42, &i); > +} > > base-commit: 713ec97e593bd4d9915a13bc4047f064fec0e24a ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v4] c++: Implement -Wself-move warning [PR81159] 2022-08-20 21:31 ` Jason Merrill @ 2022-08-23 16:39 ` Marek Polacek 2022-08-23 21:27 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-08-23 16:39 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches On Sat, Aug 20, 2022 at 05:31:52PM -0400, Jason Merrill wrote: > On 8/19/22 15:34, Marek Polacek wrote: > > On Thu, Aug 18, 2022 at 08:33:47PM -0400, Jason Merrill wrote: > > > On 8/18/22 13:19, Marek Polacek wrote: > > > > On Mon, Aug 15, 2022 at 03:54:05PM -0400, Jason Merrill wrote: > > > > > On 8/9/22 09:37, Marek Polacek wrote: > > > > > > + /* We're looking for *std::move<T&> ((T &) &arg), or > > > > > > + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ > > > > > > + if (!REFERENCE_REF_P (rhs) > > > > > > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > > > > > > + return; > > > > > > + tree fn = TREE_OPERAND (rhs, 0); > > > > > > + if (!is_std_move_p (fn)) > > > > > > + return; > > > > > > + tree arg = CALL_EXPR_ARG (fn, 0); > > > > > > + if (TREE_CODE (arg) != NOP_EXPR) > > > > > > + return; > > > > > > + /* Strip the (T &). */ > > > > > > + arg = TREE_OPERAND (arg, 0); > > > > > > + /* Strip the (T *) or &. */ > > > > > > + arg = TREE_OPERAND (arg, 0); > > > > > > > > > > Are you sure these are the only two expressions that can make it here? What > > > > > if the argument to move is *Tptr? > > > > > > > > Not 100% sure but I couldn't find any other form. For *Tptr we get > > > > *std::move<int*&> ((int * &) &Tptr) > > > > > > That likes like what you'd get when the argument is Tptr, not when it's > > > *Tptr. And indeed that's what I see in the testcase: > > > > > > > + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } > > > > > > is missing the * > > > > Duh, sorry. The previous patch didn't handle the *Tptr case. Further poking > > revealed that we need special care to handle (*(Tptr)) and **Tptr etc. So in > > this patch I'm stripping all *s and V_C_Es. Sigh. > > Ah, I was just thinking that the old patch needed more checking of > TREE_CODEs. But I suppose it's also good to diagnose these cases as well. Probably. I notice that clang++ doesn't detect it, but that doesn't mean we can't. > > + if (cp_tree_equal (lhs, arg)) > > + { > > + auto_diagnostic_group d; > > + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) > > Maybe only say "variable" when the original operand is in fact a variable? Fixed by the print_var_p thingie I introduced. I saw that clang++ points out the type so I'm printing it too in this patch. > > +// Test various other arguments to std::move. > > +template<typename T> > > +void > > +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) > > +{ > > + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } > > + *Tptr = std::move (*Tptr); // { dg-warning "moving a variable to itself" } > > + *Tptr = std::move (*(Tptr)); // { dg-warning "moving a variable to itself" } > > + *(Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } > > + *(Tptr + 1) = std::move (*Tptr + 1); > > + *(Tptr + 1) = std::move (*Tptr + 2); > > Do you mean to have *(Tptr + 1) on both sides? The difference in * operand > is surprising. Definitely not intended, good spotting! Fixed (we emit a warning for the +1 case but not the +2 case -- good). Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- About 5 years ago we got a request to implement -Wself-move, which warns about useless moves like this: int x; x = std::move (x); This patch implements that warning. PR c++/81159 gcc/c-family/ChangeLog: * c.opt (Wself-move): New option. gcc/cp/ChangeLog: * typeck.cc (maybe_warn_self_move): New. (cp_build_modify_expr): Call maybe_warn_self_move. gcc/ChangeLog: * doc/invoke.texi: Document -Wself-move. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move1.C: New test. --- gcc/c-family/c.opt | 4 + gcc/cp/typeck.cc | 61 +++++++++++- gcc/doc/invoke.texi | 23 ++++- gcc/testsuite/g++.dg/warn/Wself-move1.C | 125 ++++++++++++++++++++++++ 4 files changed, 211 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index dfdebd596ef..f776efd39d8 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1229,6 +1229,10 @@ Wselector ObjC ObjC++ Var(warn_selector) Warning Warn if a selector has multiple methods. +Wself-move +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn when a value is moved to itself with std::move. + Wsequence-point C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about possible violations of sequence point rules. diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 992ebfd99fb..194c403bd2b 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -8897,7 +8897,64 @@ cp_build_c_cast (location_t loc, tree type, tree expr, return error_mark_node; } -\f + +/* Warn when a value is moved to itself with std::move. LHS is the target, + RHS may be the std::move call, and LOC is the location of the whole + assignment. */ + +static void +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) +{ + if (!warn_self_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + if (processing_template_decl) + return; + + if (!REFERENCE_REF_P (rhs) + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) + return; + tree fn = TREE_OPERAND (rhs, 0); + if (!is_std_move_p (fn)) + return; + + /* Just a little helper to strip * and various NOPs. */ + auto extract_op = [] (tree &op) { + STRIP_NOPS (op); + while (INDIRECT_REF_P (op)) + op = TREE_OPERAND (op, 0); + op = maybe_undo_parenthesized_ref (op); + STRIP_ANY_LOCATION_WRAPPER (op); + }; + + tree arg = CALL_EXPR_ARG (fn, 0); + extract_op (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + tree type = TREE_TYPE (lhs); + lhs = maybe_undo_parenthesized_ref (lhs); + STRIP_ANY_LOCATION_WRAPPER (lhs); + const bool print_var_p = (DECL_P (lhs) + || REFERENCE_REF_P (lhs) + || TREE_CODE (lhs) == COMPONENT_REF); + extract_op (lhs); + + if (cp_tree_equal (lhs, arg)) + { + auto_diagnostic_group d; + if (print_var_p + ? warning_at (loc, OPT_Wself_move, + "moving a variable of type %qT to itself", type) + : warning_at (loc, OPT_Wself_move, + "moving an expression of type %qT to itself", type)) + inform (loc, "remove %<std::move%> call"); + } +} + /* For use from the C common bits. */ tree build_modify_expr (location_t location, @@ -9101,6 +9158,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, if (modifycode == NOP_EXPR) { + maybe_warn_self_move (loc, lhs, rhs); + if (c_dialect_objc ()) { result = objc_maybe_build_modify_expr (lhs, rhs); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 78eb7ad7e74..dc686da8b9f 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ in the following sections. -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol @@ -5844,6 +5844,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol +-Wself-move @r{(only for C++)} @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol -Wsizeof-array-div @gol @@ -6829,6 +6830,26 @@ of a declaration: This warning is enabled by @option{-Wall}. +@item -Wno-self-move @r{(C++ and Objective-C++ only)} +@opindex Wself-move +@opindex Wno-self-move +This warning warns when a value is moved to itself with @code{std::move}. +Such a @code{std::move} typically has no effect. + +@smallexample +struct T @{ +@dots{} +@}; +void fn() +@{ + T t; + @dots{} + t = std::move (t); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsequence-point @opindex Wsequence-point @opindex Wno-sequence-point diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C new file mode 100644 index 00000000000..39111f0a2b0 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C @@ -0,0 +1,125 @@ +// PR c++/81159 +// { dg-do compile { target c++11 } } +// { dg-options "-Wself-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); } +} + +int g; + +struct S { + int x; + S(S&& o) { + x = std::move (x); // { dg-warning "moving a variable of type .int. to itself" } + x = std::move (o.x); + o.x = std::move (x); + o.x = std::move (o.x); // { dg-warning "moving a variable of type .int. to itself" } + } + void foo (int x) { + x = std::move (x); // { dg-warning "moving a variable of type .int. to itself" } + } +}; + +struct X { + int x; + X(int x) : x(std::move (x)) { } +}; + +struct A {}; +struct B { A a; }; +struct C { C(); ~C(); }; +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; + +void +test () +{ + int i = 42; + i = std::move (i); // { dg-warning "moving a variable of type .int. to itself" } + (i) = std::move (i); // { dg-warning "moving a variable of type .int. to itself" } + + g = std::move (g); // { dg-warning "moving a variable of type .int. to itself" } + (g) = std::move (g); // { dg-warning "moving a variable of type .int. to itself" } + + A a; + a = std::move (a); // { dg-warning "moving a variable of type .A. to itself" } + (a) = std::move (a); // { dg-warning "moving a variable of type .A. to itself" } + + B b; + b = std::move (b); // { dg-warning "moving a variable of type .B. to itself" } + (b) = std::move (b); // { dg-warning "moving a variable of type .B. to itself" } + b.a = std::move (b.a); // { dg-warning "moving a variable of type .A. to itself" } + (b.a) = std::move (b.a); // { dg-warning "moving a variable of type .A. to itself" } + + C c; + c = std::move (c); // { dg-warning "moving a variable of type .C. to itself" } + D d; + d = std::move (d); // { dg-warning "moving a variable of type .D. to itself" } +} + +template<typename T> +void ttest () +{ + T t; + t = std::move (t); // { dg-warning "moving a variable of type .A. to itself" } +} + +template void ttest<A>(); + +void +testref (int &r, int &&rr) +{ + r = std::move (r); // { dg-warning "moving a variable of type .int. to itself" } + rr = std::move (rr); // { dg-warning "moving a variable of type .int. to itself" } +} + +// Test various other arguments to std::move. +template<typename T> +void +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) +{ + Tptr = std::move (Tptr); // { dg-warning "moving a variable of type 'int\\*' to itself" } + *Tptr = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } + *Tptr = std::move (*(Tptr)); // { dg-warning "moving an expression of type 'int' to itself" } + *(Tptr) = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } + *(Tptr + 1) = std::move (*(Tptr + 1)); // { dg-warning "moving an expression of type 'int' to itself" } + *(Tptr + 1) = std::move (*(Tptr + 2)); + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } + *Tpptr = std::move (*Tpptr); // { dg-warning "moving an expression of type 'int\\*' to itself" } + **Tpptr = std::move (**Tpptr); // { dg-warning "moving an expression of type 'int' to itself" } + Tref = std::move (Tref); // { dg-warning "moving a variable of type 'int' to itself" } + Trref = std::move (Trref); // { dg-warning "moving a variable of type 'int' to itself" } + Tcptr = std::move (Tcptr); // { dg-warning "moving a variable of type 'const int\\*' to itself" } + (Tptr) = std::move (Tptr); // { dg-warning "moving a variable of type 'int\\*' to itself" } + (*Tptr) = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving an expression of type 'int\\*' to itself" } + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving an expression of type 'int' to itself" } + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving an expression of type 'int' to itself" } + (Tref) = std::move (Tref); // { dg-warning "moving a variable of type 'int' to itself" } + (Trref) = std::move (Trref); // { dg-warning "moving a variable of type 'int' to itself" } + (Tcptr) = std::move (Tcptr); // { dg-warning "moving a variable of type 'const int\\*' to itself" } +} + +void +call_testargs () +{ + int i = 42; + int *p = &i; + testargs<int>(&i, &p, i, 42, &i); +} base-commit: baa3ffb19c54fa334ac2884f6acb5d31aa79ac32 -- 2.37.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c++: Implement -Wself-move warning [PR81159] 2022-08-23 16:39 ` [PATCH v4] " Marek Polacek @ 2022-08-23 21:27 ` Jason Merrill 2022-08-24 21:30 ` Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-08-23 21:27 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches On 8/23/22 09:39, Marek Polacek wrote: > On Sat, Aug 20, 2022 at 05:31:52PM -0400, Jason Merrill wrote: >> On 8/19/22 15:34, Marek Polacek wrote: >>> On Thu, Aug 18, 2022 at 08:33:47PM -0400, Jason Merrill wrote: >>>> On 8/18/22 13:19, Marek Polacek wrote: >>>>> On Mon, Aug 15, 2022 at 03:54:05PM -0400, Jason Merrill wrote: >>>>>> On 8/9/22 09:37, Marek Polacek wrote: >>>>>>> + /* We're looking for *std::move<T&> ((T &) &arg), or >>>>>>> + *std::move<T&> ((T &) (T *) r) if the argument it a reference. */ >>>>>>> + if (!REFERENCE_REF_P (rhs) >>>>>>> + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) >>>>>>> + return; >>>>>>> + tree fn = TREE_OPERAND (rhs, 0); >>>>>>> + if (!is_std_move_p (fn)) >>>>>>> + return; >>>>>>> + tree arg = CALL_EXPR_ARG (fn, 0); >>>>>>> + if (TREE_CODE (arg) != NOP_EXPR) >>>>>>> + return; >>>>>>> + /* Strip the (T &). */ >>>>>>> + arg = TREE_OPERAND (arg, 0); >>>>>>> + /* Strip the (T *) or &. */ >>>>>>> + arg = TREE_OPERAND (arg, 0); >>>>>> >>>>>> Are you sure these are the only two expressions that can make it here? What >>>>>> if the argument to move is *Tptr? >>>>> >>>>> Not 100% sure but I couldn't find any other form. For *Tptr we get >>>>> *std::move<int*&> ((int * &) &Tptr) >>>> >>>> That likes like what you'd get when the argument is Tptr, not when it's >>>> *Tptr. And indeed that's what I see in the testcase: >>>> >>>>> + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } >>>> >>>> is missing the * >>> >>> Duh, sorry. The previous patch didn't handle the *Tptr case. Further poking >>> revealed that we need special care to handle (*(Tptr)) and **Tptr etc. So in >>> this patch I'm stripping all *s and V_C_Es. Sigh. >> >> Ah, I was just thinking that the old patch needed more checking of >> TREE_CODEs. But I suppose it's also good to diagnose these cases as well. > > Probably. I notice that clang++ doesn't detect it, but that doesn't mean > we can't. > >>> + if (cp_tree_equal (lhs, arg)) >>> + { >>> + auto_diagnostic_group d; >>> + if (warning_at (loc, OPT_Wself_move, "moving a variable to itself")) >> >> Maybe only say "variable" when the original operand is in fact a variable? > > Fixed by the print_var_p thingie I introduced. I saw that clang++ points > out the type so I'm printing it too in this patch. > >>> +// Test various other arguments to std::move. >>> +template<typename T> >>> +void >>> +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) >>> +{ >>> + Tptr = std::move (Tptr); // { dg-warning "moving a variable to itself" } >>> + *Tptr = std::move (*Tptr); // { dg-warning "moving a variable to itself" } >>> + *Tptr = std::move (*(Tptr)); // { dg-warning "moving a variable to itself" } >>> + *(Tptr) = std::move (*Tptr); // { dg-warning "moving a variable to itself" } >>> + *(Tptr + 1) = std::move (*Tptr + 1); >>> + *(Tptr + 1) = std::move (*Tptr + 2); >> >> Do you mean to have *(Tptr + 1) on both sides? The difference in * operand >> is surprising. > > Definitely not intended, good spotting! Fixed (we emit a warning for the > +1 case but not the +2 case -- good). > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > About 5 years ago we got a request to implement -Wself-move, which > warns about useless moves like this: > > int x; > x = std::move (x); > > This patch implements that warning. > > PR c++/81159 > > gcc/c-family/ChangeLog: > > * c.opt (Wself-move): New option. > > gcc/cp/ChangeLog: > > * typeck.cc (maybe_warn_self_move): New. > (cp_build_modify_expr): Call maybe_warn_self_move. > > gcc/ChangeLog: > > * doc/invoke.texi: Document -Wself-move. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wself-move1.C: New test. > --- > gcc/c-family/c.opt | 4 + > gcc/cp/typeck.cc | 61 +++++++++++- > gcc/doc/invoke.texi | 23 ++++- > gcc/testsuite/g++.dg/warn/Wself-move1.C | 125 ++++++++++++++++++++++++ > 4 files changed, 211 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index dfdebd596ef..f776efd39d8 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -1229,6 +1229,10 @@ Wselector > ObjC ObjC++ Var(warn_selector) Warning > Warn if a selector has multiple methods. > > +Wself-move > +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) > +Warn when a value is moved to itself with std::move. > + > Wsequence-point > C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) > Warn about possible violations of sequence point rules. > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 992ebfd99fb..194c403bd2b 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -8897,7 +8897,64 @@ cp_build_c_cast (location_t loc, tree type, tree expr, > > return error_mark_node; > } > -\f > + > +/* Warn when a value is moved to itself with std::move. LHS is the target, > + RHS may be the std::move call, and LOC is the location of the whole > + assignment. */ > + > +static void > +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > +{ > + if (!warn_self_move) > + return; > + > + /* C++98 doesn't know move. */ > + if (cxx_dialect < cxx11) > + return; > + > + if (processing_template_decl) > + return; > + > + if (!REFERENCE_REF_P (rhs) > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > + return; > + tree fn = TREE_OPERAND (rhs, 0); > + if (!is_std_move_p (fn)) > + return; > + > + /* Just a little helper to strip * and various NOPs. */ > + auto extract_op = [] (tree &op) { > + STRIP_NOPS (op); > + while (INDIRECT_REF_P (op)) > + op = TREE_OPERAND (op, 0); > + op = maybe_undo_parenthesized_ref (op); > + STRIP_ANY_LOCATION_WRAPPER (op); > + }; > + > + tree arg = CALL_EXPR_ARG (fn, 0); > + extract_op (arg); > + if (TREE_CODE (arg) == ADDR_EXPR) > + arg = TREE_OPERAND (arg, 0); > + tree type = TREE_TYPE (lhs); > + lhs = maybe_undo_parenthesized_ref (lhs); > + STRIP_ANY_LOCATION_WRAPPER (lhs); > + const bool print_var_p = (DECL_P (lhs) > + || REFERENCE_REF_P (lhs) > + || TREE_CODE (lhs) == COMPONENT_REF); Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be stripped before this test, member refs aren't variables. > + extract_op (lhs); > + > + if (cp_tree_equal (lhs, arg)) > + { > + auto_diagnostic_group d; > + if (print_var_p > + ? warning_at (loc, OPT_Wself_move, > + "moving a variable of type %qT to itself", type) > + : warning_at (loc, OPT_Wself_move, > + "moving an expression of type %qT to itself", type)) > + inform (loc, "remove %<std::move%> call"); > + } > +} > + > /* For use from the C common bits. */ > tree > build_modify_expr (location_t location, > @@ -9101,6 +9158,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, > > if (modifycode == NOP_EXPR) > { > + maybe_warn_self_move (loc, lhs, rhs); > + > if (c_dialect_objc ()) > { > result = objc_maybe_build_modify_expr (lhs, rhs); > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 78eb7ad7e74..dc686da8b9f 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -264,7 +264,7 @@ in the following sections. > -Wreorder -Wregister @gol > -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol > -Wno-non-template-friend -Wold-style-cast @gol > --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol > -Wsized-deallocation -Wsuggest-final-methods @gol > -Wsuggest-final-types -Wsuggest-override @gol > -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > @@ -5844,6 +5844,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. > -Wreorder @gol > -Wrestrict @gol > -Wreturn-type @gol > +-Wself-move @r{(only for C++)} @gol > -Wsequence-point @gol > -Wsign-compare @r{(only in C++)} @gol > -Wsizeof-array-div @gol > @@ -6829,6 +6830,26 @@ of a declaration: > > This warning is enabled by @option{-Wall}. > > +@item -Wno-self-move @r{(C++ and Objective-C++ only)} > +@opindex Wself-move > +@opindex Wno-self-move > +This warning warns when a value is moved to itself with @code{std::move}. > +Such a @code{std::move} typically has no effect. > + > +@smallexample > +struct T @{ > +@dots{} > +@}; > +void fn() > +@{ > + T t; > + @dots{} > + t = std::move (t); > +@} > +@end smallexample > + > +This warning is enabled by @option{-Wall}. > + > @item -Wsequence-point > @opindex Wsequence-point > @opindex Wno-sequence-point > diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C > new file mode 100644 > index 00000000000..39111f0a2b0 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C > @@ -0,0 +1,125 @@ > +// PR c++/81159 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wself-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); } > +} > + > +int g; > + > +struct S { > + int x; > + S(S&& o) { > + x = std::move (x); // { dg-warning "moving a variable of type .int. to itself" } > + x = std::move (o.x); > + o.x = std::move (x); > + o.x = std::move (o.x); // { dg-warning "moving a variable of type .int. to itself" } > + } > + void foo (int x) { > + x = std::move (x); // { dg-warning "moving a variable of type .int. to itself" } > + } > +}; > + > +struct X { > + int x; > + X(int x) : x(std::move (x)) { } > +}; > + > +struct A {}; > +struct B { A a; }; > +struct C { C(); ~C(); }; > +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; > + > +void > +test () > +{ > + int i = 42; > + i = std::move (i); // { dg-warning "moving a variable of type .int. to itself" } > + (i) = std::move (i); // { dg-warning "moving a variable of type .int. to itself" } > + > + g = std::move (g); // { dg-warning "moving a variable of type .int. to itself" } > + (g) = std::move (g); // { dg-warning "moving a variable of type .int. to itself" } > + > + A a; > + a = std::move (a); // { dg-warning "moving a variable of type .A. to itself" } > + (a) = std::move (a); // { dg-warning "moving a variable of type .A. to itself" } > + > + B b; > + b = std::move (b); // { dg-warning "moving a variable of type .B. to itself" } > + (b) = std::move (b); // { dg-warning "moving a variable of type .B. to itself" } > + b.a = std::move (b.a); // { dg-warning "moving a variable of type .A. to itself" } > + (b.a) = std::move (b.a); // { dg-warning "moving a variable of type .A. to itself" } > + > + C c; > + c = std::move (c); // { dg-warning "moving a variable of type .C. to itself" } > + D d; > + d = std::move (d); // { dg-warning "moving a variable of type .D. to itself" } > +} > + > +template<typename T> > +void ttest () > +{ > + T t; > + t = std::move (t); // { dg-warning "moving a variable of type .A. to itself" } > +} > + > +template void ttest<A>(); > + > +void > +testref (int &r, int &&rr) > +{ > + r = std::move (r); // { dg-warning "moving a variable of type .int. to itself" } > + rr = std::move (rr); // { dg-warning "moving a variable of type .int. to itself" } > +} > + > +// Test various other arguments to std::move. > +template<typename T> > +void > +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) > +{ > + Tptr = std::move (Tptr); // { dg-warning "moving a variable of type 'int\\*' to itself" } > + *Tptr = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } > + *Tptr = std::move (*(Tptr)); // { dg-warning "moving an expression of type 'int' to itself" } > + *(Tptr) = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } > + *(Tptr + 1) = std::move (*(Tptr + 1)); // { dg-warning "moving an expression of type 'int' to itself" } > + *(Tptr + 1) = std::move (*(Tptr + 2)); > + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } > + *Tpptr = std::move (*Tpptr); // { dg-warning "moving an expression of type 'int\\*' to itself" } > + **Tpptr = std::move (**Tpptr); // { dg-warning "moving an expression of type 'int' to itself" } > + Tref = std::move (Tref); // { dg-warning "moving a variable of type 'int' to itself" } > + Trref = std::move (Trref); // { dg-warning "moving a variable of type 'int' to itself" } > + Tcptr = std::move (Tcptr); // { dg-warning "moving a variable of type 'const int\\*' to itself" } > + (Tptr) = std::move (Tptr); // { dg-warning "moving a variable of type 'int\\*' to itself" } > + (*Tptr) = std::move (*Tptr); // { dg-warning "moving an expression of type 'int' to itself" } > + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving an expression of type 'int\\*' to itself" } > + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving an expression of type 'int' to itself" } > + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving an expression of type 'int' to itself" } > + (Tref) = std::move (Tref); // { dg-warning "moving a variable of type 'int' to itself" } > + (Trref) = std::move (Trref); // { dg-warning "moving a variable of type 'int' to itself" } > + (Tcptr) = std::move (Tcptr); // { dg-warning "moving a variable of type 'const int\\*' to itself" } > +} > + > +void > +call_testargs () > +{ > + int i = 42; > + int *p = &i; > + testargs<int>(&i, &p, i, 42, &i); > +} > > base-commit: baa3ffb19c54fa334ac2884f6acb5d31aa79ac32 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c++: Implement -Wself-move warning [PR81159] 2022-08-23 21:27 ` Jason Merrill @ 2022-08-24 21:30 ` Marek Polacek 2022-08-25 13:25 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-08-24 21:30 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches On Tue, Aug 23, 2022 at 05:27:00PM -0400, Jason Merrill wrote: > On 8/23/22 09:39, Marek Polacek wrote: > > + tree arg = CALL_EXPR_ARG (fn, 0); > > + extract_op (arg); > > + if (TREE_CODE (arg) == ADDR_EXPR) > > + arg = TREE_OPERAND (arg, 0); > > + tree type = TREE_TYPE (lhs); > > + lhs = maybe_undo_parenthesized_ref (lhs); > > + STRIP_ANY_LOCATION_WRAPPER (lhs); > > + const bool print_var_p = (DECL_P (lhs) > > + || REFERENCE_REF_P (lhs) > > + || TREE_CODE (lhs) == COMPONENT_REF); > > Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be > stripped before this test, member refs aren't variables. I'm checking REFERENCE_REF_P and COMPONENT_REF to say "moving a variable" in #1 and #3. The REFERENCE_REF_P check means that we also say "variable" for #2. Sure, "A variable is introduced by the declaration of a reference other than a non-static data member", but I'm not sure if users care about that here? If I strip REFERENCE_REFs before the check then the result will be the same. Or I could keep only the DECL_P check, but then we'll say "moving an expression" for #1 and #2, which seems strange. struct S { int x; int &r; void foo () { x = std::move (x); // #1 r = std::move (r); // #2 }; }; void foo (int &r) { r = std::move (r); // #3 } Marek ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c++: Implement -Wself-move warning [PR81159] 2022-08-24 21:30 ` Marek Polacek @ 2022-08-25 13:25 ` Jason Merrill 2022-08-25 21:49 ` Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-08-25 13:25 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches On 8/24/22 17:30, Marek Polacek wrote: > On Tue, Aug 23, 2022 at 05:27:00PM -0400, Jason Merrill wrote: >> On 8/23/22 09:39, Marek Polacek wrote: >>> + tree arg = CALL_EXPR_ARG (fn, 0); >>> + extract_op (arg); >>> + if (TREE_CODE (arg) == ADDR_EXPR) >>> + arg = TREE_OPERAND (arg, 0); >>> + tree type = TREE_TYPE (lhs); >>> + lhs = maybe_undo_parenthesized_ref (lhs); >>> + STRIP_ANY_LOCATION_WRAPPER (lhs); >>> + const bool print_var_p = (DECL_P (lhs) >>> + || REFERENCE_REF_P (lhs) >>> + || TREE_CODE (lhs) == COMPONENT_REF); >> >> Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be >> stripped before this test, member refs aren't variables. > > I'm checking REFERENCE_REF_P and COMPONENT_REF to say "moving a variable" > in #1 and #3. The REFERENCE_REF_P check means that we also say "variable" > for #2. Sure, "A variable is introduced by the declaration of a reference > other than a non-static data member", but I'm not sure if users care about > that here? > > If I strip REFERENCE_REFs before the check then the result will be the > same. That's what I was suggesting, yes: Strip the REFERENCE_REF so DECL_P can see the decl. I don't see where COMPONENT_REF comes in? > Or I could keep only the DECL_P check, but then we'll say "moving > an expression" for #1 and #2, which seems strange. > > struct S { > int x; > int &r; > void foo () { > x = std::move (x); // #1 > r = std::move (r); // #2 > }; > }; > > void > foo (int &r) > { > r = std::move (r); // #3 > } > > Marek > ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c++: Implement -Wself-move warning [PR81159] 2022-08-25 13:25 ` Jason Merrill @ 2022-08-25 21:49 ` Marek Polacek 2022-08-26 0:52 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-08-25 21:49 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches On Thu, Aug 25, 2022 at 09:25:43AM -0400, Jason Merrill wrote: > On 8/24/22 17:30, Marek Polacek wrote: > > On Tue, Aug 23, 2022 at 05:27:00PM -0400, Jason Merrill wrote: > > > On 8/23/22 09:39, Marek Polacek wrote: > > > > + tree arg = CALL_EXPR_ARG (fn, 0); > > > > + extract_op (arg); > > > > + if (TREE_CODE (arg) == ADDR_EXPR) > > > > + arg = TREE_OPERAND (arg, 0); > > > > + tree type = TREE_TYPE (lhs); > > > > + lhs = maybe_undo_parenthesized_ref (lhs); > > > > + STRIP_ANY_LOCATION_WRAPPER (lhs); > > > > + const bool print_var_p = (DECL_P (lhs) > > > > + || REFERENCE_REF_P (lhs) > > > > + || TREE_CODE (lhs) == COMPONENT_REF); > > > > > > Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be > > > stripped before this test, member refs aren't variables. > > > > I'm checking REFERENCE_REF_P and COMPONENT_REF to say "moving a variable" > > in #1 and #3. The REFERENCE_REF_P check means that we also say "variable" > > for #2. Sure, "A variable is introduced by the declaration of a reference > > other than a non-static data member", but I'm not sure if users care about > > that here? > > > > If I strip REFERENCE_REFs before the check then the result will be the > > same. > > That's what I was suggesting, yes: Strip the REFERENCE_REF so DECL_P can see > the decl. Ok, I've added the REFERENCE_REF stripping. But I've still left the COMPONENT_REF in. Perhaps we could say "moving a member" to itself for COMPONENT_REFs. Or just say "moving 'x' of type 'int' to itself" and avoid all of this. :) > I don't see where COMPONENT_REF comes in? For #1 in the test below the COMPONENT_REF was created in finish_id_expression -> finish_non_static_data_member -> build_class_member_access_expr and passed down to maybe_warn_self_move from here: #0 maybe_warn_self_move (loc=2147483652, lhs=<component_ref 0x7fffea380e10>, rhs=<indirect_ref 0x7fffea38a220>) at /home/mpolacek/src/gcc/gcc/cp/typeck.cc:8908 #1 0x0000000000f3d03e in cp_build_modify_expr (loc=2147483652, lhs=<component_ref 0x7fffea380e10>, modifycode=NOP_EXPR, rhs=<indirect_ref 0x7fffea38a220>, complain=3) at /home/mpolacek/src/gcc/gcc/cp/typeck.cc:9161 #2 0x0000000000f3e461 in build_x_modify_expr (loc=2147483652, lhs=<component_ref 0x7fffea380e10>, modifycode=NOP_EXPR, rhs=<indirect_ref 0x7fffea38a220>, lookups=<tree 0x0>, complain=3) at /home/mpolacek/src/gcc/gcc/cp/typeck.cc:9446 #3 0x0000000000d92d4e in cp_parser_assignment_expression (parser=0x7fffea236850, pidk=0x0, cast_p=false, decltype_p=false) at /home/mpolacek/src/gcc/gcc/cp/parser.cc:10461 > > Or I could keep only the DECL_P check, but then we'll say "moving > > an expression" for #1 and #2, which seems strange. > > > > struct S { > > int x; > > int &r; > > void foo () { > > x = std::move (x); // #1 > > r = std::move (r); // #2 > > }; > > }; > > > > void > > foo (int &r) > > { > > r = std::move (r); // #3 > > } Marek ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v4] c++: Implement -Wself-move warning [PR81159] 2022-08-25 21:49 ` Marek Polacek @ 2022-08-26 0:52 ` Jason Merrill 2022-08-26 17:04 ` [PATCH v5] " Marek Polacek 0 siblings, 1 reply; 14+ messages in thread From: Jason Merrill @ 2022-08-26 0:52 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches On 8/25/22 17:49, Marek Polacek wrote: > On Thu, Aug 25, 2022 at 09:25:43AM -0400, Jason Merrill wrote: >> On 8/24/22 17:30, Marek Polacek wrote: >>> On Tue, Aug 23, 2022 at 05:27:00PM -0400, Jason Merrill wrote: >>>> On 8/23/22 09:39, Marek Polacek wrote: >>>>> + tree arg = CALL_EXPR_ARG (fn, 0); >>>>> + extract_op (arg); >>>>> + if (TREE_CODE (arg) == ADDR_EXPR) >>>>> + arg = TREE_OPERAND (arg, 0); >>>>> + tree type = TREE_TYPE (lhs); >>>>> + lhs = maybe_undo_parenthesized_ref (lhs); >>>>> + STRIP_ANY_LOCATION_WRAPPER (lhs); >>>>> + const bool print_var_p = (DECL_P (lhs) >>>>> + || REFERENCE_REF_P (lhs) >>>>> + || TREE_CODE (lhs) == COMPONENT_REF); >>>> >>>> Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be >>>> stripped before this test, member refs aren't variables. >>> >>> I'm checking REFERENCE_REF_P and COMPONENT_REF to say "moving a variable" >>> in #1 and #3. The REFERENCE_REF_P check means that we also say "variable" >>> for #2. Sure, "A variable is introduced by the declaration of a reference >>> other than a non-static data member", but I'm not sure if users care about >>> that here? >>> >>> If I strip REFERENCE_REFs before the check then the result will be the >>> same. >> >> That's what I was suggesting, yes: Strip the REFERENCE_REF so DECL_P can see >> the decl. > > Ok, I've added the REFERENCE_REF stripping. But I've still left the > COMPONENT_REF in. Perhaps we could say "moving a member" to itself for > COMPONENT_REFs. Or just say "moving 'x' of type 'int' to itself" and > avoid all of this. :) Sure, that would be simpler. In any case, we shouldn't call member references variables. Jason ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v5] c++: Implement -Wself-move warning [PR81159] 2022-08-26 0:52 ` Jason Merrill @ 2022-08-26 17:04 ` Marek Polacek 2022-08-26 17:59 ` Jason Merrill 0 siblings, 1 reply; 14+ messages in thread From: Marek Polacek @ 2022-08-26 17:04 UTC (permalink / raw) To: Jason Merrill; +Cc: GCC Patches On Thu, Aug 25, 2022 at 08:52:58PM -0400, Jason Merrill wrote: > On 8/25/22 17:49, Marek Polacek wrote: > > On Thu, Aug 25, 2022 at 09:25:43AM -0400, Jason Merrill wrote: > > > On 8/24/22 17:30, Marek Polacek wrote: > > > > On Tue, Aug 23, 2022 at 05:27:00PM -0400, Jason Merrill wrote: > > > > > On 8/23/22 09:39, Marek Polacek wrote: > > > > > > + tree arg = CALL_EXPR_ARG (fn, 0); > > > > > > + extract_op (arg); > > > > > > + if (TREE_CODE (arg) == ADDR_EXPR) > > > > > > + arg = TREE_OPERAND (arg, 0); > > > > > > + tree type = TREE_TYPE (lhs); > > > > > > + lhs = maybe_undo_parenthesized_ref (lhs); > > > > > > + STRIP_ANY_LOCATION_WRAPPER (lhs); > > > > > > + const bool print_var_p = (DECL_P (lhs) > > > > > > + || REFERENCE_REF_P (lhs) > > > > > > + || TREE_CODE (lhs) == COMPONENT_REF); > > > > > > > > > > Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be > > > > > stripped before this test, member refs aren't variables. > > > > > > > > I'm checking REFERENCE_REF_P and COMPONENT_REF to say "moving a variable" > > > > in #1 and #3. The REFERENCE_REF_P check means that we also say "variable" > > > > for #2. Sure, "A variable is introduced by the declaration of a reference > > > > other than a non-static data member", but I'm not sure if users care about > > > > that here? > > > > > > > > If I strip REFERENCE_REFs before the check then the result will be the > > > > same. > > > > > > That's what I was suggesting, yes: Strip the REFERENCE_REF so DECL_P can see > > > the decl. > > > > Ok, I've added the REFERENCE_REF stripping. But I've still left the > > COMPONENT_REF in. Perhaps we could say "moving a member" to itself for > > COMPONENT_REFs. Or just say "moving 'x' of type 'int' to itself" and > > avoid all of this. :) > > Sure, that would be simpler. In any case, we shouldn't call member > references variables. Here's the "moving 'x' of type 'int' to itself" version then: Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? -- >8 -- About 5 years ago we got a request to implement -Wself-move, which warns about useless moves like this: int x; x = std::move (x); This patch implements that warning. PR c++/81159 gcc/c-family/ChangeLog: * c.opt (Wself-move): New option. gcc/cp/ChangeLog: * typeck.cc (maybe_warn_self_move): New. (cp_build_modify_expr): Call maybe_warn_self_move. gcc/ChangeLog: * doc/invoke.texi: Document -Wself-move. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move1.C: New test. --- gcc/c-family/c.opt | 4 + gcc/cp/typeck.cc | 53 +++++++++- gcc/doc/invoke.texi | 23 ++++- gcc/testsuite/g++.dg/warn/Wself-move1.C | 125 ++++++++++++++++++++++++ 4 files changed, 203 insertions(+), 2 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index dfdebd596ef..f776efd39d8 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -1229,6 +1229,10 @@ Wselector ObjC ObjC++ Var(warn_selector) Warning Warn if a selector has multiple methods. +Wself-move +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) +Warn when a value is moved to itself with std::move. + Wsequence-point C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) Warn about possible violations of sequence point rules. diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 7fde65adaa4..b99947c10fd 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -8897,7 +8897,56 @@ cp_build_c_cast (location_t loc, tree type, tree expr, return error_mark_node; } -\f + +/* Warn when a value is moved to itself with std::move. LHS is the target, + RHS may be the std::move call, and LOC is the location of the whole + assignment. */ + +static void +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) +{ + if (!warn_self_move) + return; + + /* C++98 doesn't know move. */ + if (cxx_dialect < cxx11) + return; + + if (processing_template_decl) + return; + + if (!REFERENCE_REF_P (rhs) + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) + return; + tree fn = TREE_OPERAND (rhs, 0); + if (!is_std_move_p (fn)) + return; + + /* Just a little helper to strip * and various NOPs. */ + auto extract_op = [] (tree &op) { + STRIP_NOPS (op); + while (INDIRECT_REF_P (op)) + op = TREE_OPERAND (op, 0); + op = maybe_undo_parenthesized_ref (op); + STRIP_ANY_LOCATION_WRAPPER (op); + }; + + tree arg = CALL_EXPR_ARG (fn, 0); + extract_op (arg); + if (TREE_CODE (arg) == ADDR_EXPR) + arg = TREE_OPERAND (arg, 0); + tree type = TREE_TYPE (lhs); + tree orig_lhs = lhs; + extract_op (lhs); + if (cp_tree_equal (lhs, arg)) + { + auto_diagnostic_group d; + if (warning_at (loc, OPT_Wself_move, + "moving %qE of type %qT to itself", orig_lhs, type)) + inform (loc, "remove %<std::move%> call"); + } +} + /* For use from the C common bits. */ tree build_modify_expr (location_t location, @@ -9101,6 +9150,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, if (modifycode == NOP_EXPR) { + maybe_warn_self_move (loc, lhs, rhs); + if (c_dialect_objc ()) { result = objc_maybe_build_modify_expr (lhs, rhs); diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 9ba83a6b1f3..6131bfa7acf 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -264,7 +264,7 @@ in the following sections. -Wreorder -Wregister @gol -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol -Wno-non-template-friend -Wold-style-cast @gol --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol -Wsized-deallocation -Wsuggest-final-methods @gol -Wsuggest-final-types -Wsuggest-override @gol -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol @@ -5838,6 +5838,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wreorder @gol -Wrestrict @gol -Wreturn-type @gol +-Wself-move @r{(only for C++)} @gol -Wsequence-point @gol -Wsign-compare @r{(only in C++)} @gol -Wsizeof-array-div @gol @@ -6823,6 +6824,26 @@ of a declaration: This warning is enabled by @option{-Wall}. +@item -Wno-self-move @r{(C++ and Objective-C++ only)} +@opindex Wself-move +@opindex Wno-self-move +This warning warns when a value is moved to itself with @code{std::move}. +Such a @code{std::move} typically has no effect. + +@smallexample +struct T @{ +@dots{} +@}; +void fn() +@{ + T t; + @dots{} + t = std::move (t); +@} +@end smallexample + +This warning is enabled by @option{-Wall}. + @item -Wsequence-point @opindex Wsequence-point @opindex Wno-sequence-point diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C new file mode 100644 index 00000000000..5c9fc92c347 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C @@ -0,0 +1,125 @@ +// PR c++/81159 +// { dg-do compile { target c++11 } } +// { dg-options "-Wself-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); } +} + +int g; + +struct S { + int x; + S(S&& o) { + x = std::move (x); // { dg-warning "moving '\[^\n\r]*S::x' of type .int. to itself" } + x = std::move (o.x); + o.x = std::move (x); + o.x = std::move (o.x); // { dg-warning "moving 'o.S::x' of type .int. to itself" } + } + void foo (int x) { + x = std::move (x); // { dg-warning "moving 'x' of type .int. to itself" } + } +}; + +struct X { + int x; + X(int x) : x(std::move (x)) { } +}; + +struct A {}; +struct B { A a; }; +struct C { C(); ~C(); }; +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; + +void +test () +{ + int i = 42; + i = std::move (i); // { dg-warning "moving 'i' of type .int. to itself" } + (i) = std::move (i); // { dg-warning "moving 'i' of type .int. to itself" } + + g = std::move (g); // { dg-warning "moving 'g' of type .int. to itself" } + (g) = std::move (g); // { dg-warning "moving 'g' of type .int. to itself" } + + A a; + a = std::move (a); // { dg-warning "moving 'a' of type .A. to itself" } + (a) = std::move (a); // { dg-warning "moving 'a' of type .A. to itself" } + + B b; + b = std::move (b); // { dg-warning "moving 'b' of type .B. to itself" } + (b) = std::move (b); // { dg-warning "moving 'b' of type .B. to itself" } + b.a = std::move (b.a); // { dg-warning "moving 'b.B::a' of type .A. to itself" } + (b.a) = std::move (b.a); // { dg-warning "moving 'b.B::a' of type .A. to itself" } + + C c; + c = std::move (c); // { dg-warning "moving 'c' of type .C. to itself" } + D d; + d = std::move (d); // { dg-warning "moving 'd' of type .D. to itself" } +} + +template<typename T> +void ttest () +{ + T t; + t = std::move (t); // { dg-warning "moving 't' of type .A. to itself" } +} + +template void ttest<A>(); + +void +testref (int &r, int &&rr) +{ + r = std::move (r); // { dg-warning "moving 'r' of type .int. to itself" } + rr = std::move (rr); // { dg-warning "moving 'rr' of type .int. to itself" } +} + +// Test various other arguments to std::move. +template<typename T> +void +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) +{ + Tptr = std::move (Tptr); // { dg-warning "moving 'Tptr' of type 'int\\*' to itself" } + *Tptr = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *Tptr = std::move (*(Tptr)); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *(Tptr) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *(Tptr + 1) = std::move (*(Tptr + 1)); // { dg-warning "moving '\[^\n\r]*Tptr\[^\n\r]*' of type 'int' to itself" } + *(Tptr + 1) = std::move (*(Tptr + 2)); + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + *Tpptr = std::move (*Tpptr); // { dg-warning "moving '\\* Tpptr' of type 'int\\*' to itself" } + **Tpptr = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } + Tref = std::move (Tref); // { dg-warning "moving 'Tref' of type 'int' to itself" } + Trref = std::move (Trref); // { dg-warning "moving 'Trref' of type 'int' to itself" } + Tcptr = std::move (Tcptr); // { dg-warning "moving 'Tcptr' of type 'const int\\*' to itself" } + (Tptr) = std::move (Tptr); // { dg-warning "moving 'Tptr' of type 'int\\*' to itself" } + (*Tptr) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving '\\* Tpptr' of type 'int\\*' to itself" } + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } + (Tref) = std::move (Tref); // { dg-warning "moving 'Tref' of type 'int' to itself" } + (Trref) = std::move (Trref); // { dg-warning "moving 'Trref' of type 'int' to itself" } + (Tcptr) = std::move (Tcptr); // { dg-warning "moving 'Tcptr' of type 'const int\\*' to itself" } +} + +void +call_testargs () +{ + int i = 42; + int *p = &i; + testargs<int>(&i, &p, i, 42, &i); +} base-commit: 1e2462890ac748a5e5b6a34fdeb61e7027863a90 -- 2.37.2 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v5] c++: Implement -Wself-move warning [PR81159] 2022-08-26 17:04 ` [PATCH v5] " Marek Polacek @ 2022-08-26 17:59 ` Jason Merrill 0 siblings, 0 replies; 14+ messages in thread From: Jason Merrill @ 2022-08-26 17:59 UTC (permalink / raw) To: Marek Polacek; +Cc: GCC Patches On 8/26/22 13:04, Marek Polacek wrote: > On Thu, Aug 25, 2022 at 08:52:58PM -0400, Jason Merrill wrote: >> On 8/25/22 17:49, Marek Polacek wrote: >>> On Thu, Aug 25, 2022 at 09:25:43AM -0400, Jason Merrill wrote: >>>> On 8/24/22 17:30, Marek Polacek wrote: >>>>> On Tue, Aug 23, 2022 at 05:27:00PM -0400, Jason Merrill wrote: >>>>>> On 8/23/22 09:39, Marek Polacek wrote: >>>>>>> + tree arg = CALL_EXPR_ARG (fn, 0); >>>>>>> + extract_op (arg); >>>>>>> + if (TREE_CODE (arg) == ADDR_EXPR) >>>>>>> + arg = TREE_OPERAND (arg, 0); >>>>>>> + tree type = TREE_TYPE (lhs); >>>>>>> + lhs = maybe_undo_parenthesized_ref (lhs); >>>>>>> + STRIP_ANY_LOCATION_WRAPPER (lhs); >>>>>>> + const bool print_var_p = (DECL_P (lhs) >>>>>>> + || REFERENCE_REF_P (lhs) >>>>>>> + || TREE_CODE (lhs) == COMPONENT_REF); >>>>>> >>>>>> Why include REFERENCE_REF_P and COMPONENT_REF? Reference refs should be >>>>>> stripped before this test, member refs aren't variables. >>>>> >>>>> I'm checking REFERENCE_REF_P and COMPONENT_REF to say "moving a variable" >>>>> in #1 and #3. The REFERENCE_REF_P check means that we also say "variable" >>>>> for #2. Sure, "A variable is introduced by the declaration of a reference >>>>> other than a non-static data member", but I'm not sure if users care about >>>>> that here? >>>>> >>>>> If I strip REFERENCE_REFs before the check then the result will be the >>>>> same. >>>> >>>> That's what I was suggesting, yes: Strip the REFERENCE_REF so DECL_P can see >>>> the decl. >>> >>> Ok, I've added the REFERENCE_REF stripping. But I've still left the >>> COMPONENT_REF in. Perhaps we could say "moving a member" to itself for >>> COMPONENT_REFs. Or just say "moving 'x' of type 'int' to itself" and >>> avoid all of this. :) >> >> Sure, that would be simpler. In any case, we shouldn't call member >> references variables. > > Here's the "moving 'x' of type 'int' to itself" version then: > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? OK. > -- >8 -- > About 5 years ago we got a request to implement -Wself-move, which > warns about useless moves like this: > > int x; > x = std::move (x); > > This patch implements that warning. > > PR c++/81159 > > gcc/c-family/ChangeLog: > > * c.opt (Wself-move): New option. > > gcc/cp/ChangeLog: > > * typeck.cc (maybe_warn_self_move): New. > (cp_build_modify_expr): Call maybe_warn_self_move. > > gcc/ChangeLog: > > * doc/invoke.texi: Document -Wself-move. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wself-move1.C: New test. > --- > gcc/c-family/c.opt | 4 + > gcc/cp/typeck.cc | 53 +++++++++- > gcc/doc/invoke.texi | 23 ++++- > gcc/testsuite/g++.dg/warn/Wself-move1.C | 125 ++++++++++++++++++++++++ > 4 files changed, 203 insertions(+), 2 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move1.C > > diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt > index dfdebd596ef..f776efd39d8 100644 > --- a/gcc/c-family/c.opt > +++ b/gcc/c-family/c.opt > @@ -1229,6 +1229,10 @@ Wselector > ObjC ObjC++ Var(warn_selector) Warning > Warn if a selector has multiple methods. > > +Wself-move > +C++ ObjC++ Var(warn_self_move) Warning LangEnabledBy(C++ ObjC++, Wall) > +Warn when a value is moved to itself with std::move. > + > Wsequence-point > C ObjC C++ ObjC++ Var(warn_sequence_point) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) > Warn about possible violations of sequence point rules. > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index 7fde65adaa4..b99947c10fd 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -8897,7 +8897,56 @@ cp_build_c_cast (location_t loc, tree type, tree expr, > > return error_mark_node; > } > -\f > + > +/* Warn when a value is moved to itself with std::move. LHS is the target, > + RHS may be the std::move call, and LOC is the location of the whole > + assignment. */ > + > +static void > +maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > +{ > + if (!warn_self_move) > + return; > + > + /* C++98 doesn't know move. */ > + if (cxx_dialect < cxx11) > + return; > + > + if (processing_template_decl) > + return; > + > + if (!REFERENCE_REF_P (rhs) > + || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > + return; > + tree fn = TREE_OPERAND (rhs, 0); > + if (!is_std_move_p (fn)) > + return; > + > + /* Just a little helper to strip * and various NOPs. */ > + auto extract_op = [] (tree &op) { > + STRIP_NOPS (op); > + while (INDIRECT_REF_P (op)) > + op = TREE_OPERAND (op, 0); > + op = maybe_undo_parenthesized_ref (op); > + STRIP_ANY_LOCATION_WRAPPER (op); > + }; > + > + tree arg = CALL_EXPR_ARG (fn, 0); > + extract_op (arg); > + if (TREE_CODE (arg) == ADDR_EXPR) > + arg = TREE_OPERAND (arg, 0); > + tree type = TREE_TYPE (lhs); > + tree orig_lhs = lhs; > + extract_op (lhs); > + if (cp_tree_equal (lhs, arg)) > + { > + auto_diagnostic_group d; > + if (warning_at (loc, OPT_Wself_move, > + "moving %qE of type %qT to itself", orig_lhs, type)) > + inform (loc, "remove %<std::move%> call"); > + } > +} > + > /* For use from the C common bits. */ > tree > build_modify_expr (location_t location, > @@ -9101,6 +9150,8 @@ cp_build_modify_expr (location_t loc, tree lhs, enum tree_code modifycode, > > if (modifycode == NOP_EXPR) > { > + maybe_warn_self_move (loc, lhs, rhs); > + > if (c_dialect_objc ()) > { > result = objc_maybe_build_modify_expr (lhs, rhs); > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 9ba83a6b1f3..6131bfa7acf 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -264,7 +264,7 @@ in the following sections. > -Wreorder -Wregister @gol > -Wstrict-null-sentinel -Wno-subobject-linkage -Wtemplates @gol > -Wno-non-template-friend -Wold-style-cast @gol > --Woverloaded-virtual -Wno-pmf-conversions -Wsign-promo @gol > +-Woverloaded-virtual -Wno-pmf-conversions -Wself-move -Wsign-promo @gol > -Wsized-deallocation -Wsuggest-final-methods @gol > -Wsuggest-final-types -Wsuggest-override @gol > -Wno-terminate -Wuseless-cast -Wno-vexing-parse @gol > @@ -5838,6 +5838,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. > -Wreorder @gol > -Wrestrict @gol > -Wreturn-type @gol > +-Wself-move @r{(only for C++)} @gol > -Wsequence-point @gol > -Wsign-compare @r{(only in C++)} @gol > -Wsizeof-array-div @gol > @@ -6823,6 +6824,26 @@ of a declaration: > > This warning is enabled by @option{-Wall}. > > +@item -Wno-self-move @r{(C++ and Objective-C++ only)} > +@opindex Wself-move > +@opindex Wno-self-move > +This warning warns when a value is moved to itself with @code{std::move}. > +Such a @code{std::move} typically has no effect. > + > +@smallexample > +struct T @{ > +@dots{} > +@}; > +void fn() > +@{ > + T t; > + @dots{} > + t = std::move (t); > +@} > +@end smallexample > + > +This warning is enabled by @option{-Wall}. > + > @item -Wsequence-point > @opindex Wsequence-point > @opindex Wno-sequence-point > diff --git a/gcc/testsuite/g++.dg/warn/Wself-move1.C b/gcc/testsuite/g++.dg/warn/Wself-move1.C > new file mode 100644 > index 00000000000..5c9fc92c347 > --- /dev/null > +++ b/gcc/testsuite/g++.dg/warn/Wself-move1.C > @@ -0,0 +1,125 @@ > +// PR c++/81159 > +// { dg-do compile { target c++11 } } > +// { dg-options "-Wself-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); } > +} > + > +int g; > + > +struct S { > + int x; > + S(S&& o) { > + x = std::move (x); // { dg-warning "moving '\[^\n\r]*S::x' of type .int. to itself" } > + x = std::move (o.x); > + o.x = std::move (x); > + o.x = std::move (o.x); // { dg-warning "moving 'o.S::x' of type .int. to itself" } > + } > + void foo (int x) { > + x = std::move (x); // { dg-warning "moving 'x' of type .int. to itself" } > + } > +}; > + > +struct X { > + int x; > + X(int x) : x(std::move (x)) { } > +}; > + > +struct A {}; > +struct B { A a; }; > +struct C { C(); ~C(); }; > +struct D { D(); D(const D&); D(D&&); D& operator=(const D&); }; > + > +void > +test () > +{ > + int i = 42; > + i = std::move (i); // { dg-warning "moving 'i' of type .int. to itself" } > + (i) = std::move (i); // { dg-warning "moving 'i' of type .int. to itself" } > + > + g = std::move (g); // { dg-warning "moving 'g' of type .int. to itself" } > + (g) = std::move (g); // { dg-warning "moving 'g' of type .int. to itself" } > + > + A a; > + a = std::move (a); // { dg-warning "moving 'a' of type .A. to itself" } > + (a) = std::move (a); // { dg-warning "moving 'a' of type .A. to itself" } > + > + B b; > + b = std::move (b); // { dg-warning "moving 'b' of type .B. to itself" } > + (b) = std::move (b); // { dg-warning "moving 'b' of type .B. to itself" } > + b.a = std::move (b.a); // { dg-warning "moving 'b.B::a' of type .A. to itself" } > + (b.a) = std::move (b.a); // { dg-warning "moving 'b.B::a' of type .A. to itself" } > + > + C c; > + c = std::move (c); // { dg-warning "moving 'c' of type .C. to itself" } > + D d; > + d = std::move (d); // { dg-warning "moving 'd' of type .D. to itself" } > +} > + > +template<typename T> > +void ttest () > +{ > + T t; > + t = std::move (t); // { dg-warning "moving 't' of type .A. to itself" } > +} > + > +template void ttest<A>(); > + > +void > +testref (int &r, int &&rr) > +{ > + r = std::move (r); // { dg-warning "moving 'r' of type .int. to itself" } > + rr = std::move (rr); // { dg-warning "moving 'rr' of type .int. to itself" } > +} > + > +// Test various other arguments to std::move. > +template<typename T> > +void > +testargs (T *Tptr, T **Tpptr, T& Tref, T&& Trref, const T *Tcptr) > +{ > + Tptr = std::move (Tptr); // { dg-warning "moving 'Tptr' of type 'int\\*' to itself" } > + *Tptr = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } > + *Tptr = std::move (*(Tptr)); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } > + *(Tptr) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } > + *(Tptr + 1) = std::move (*(Tptr + 1)); // { dg-warning "moving '\[^\n\r]*Tptr\[^\n\r]*' of type 'int' to itself" } > + *(Tptr + 1) = std::move (*(Tptr + 2)); > + (*(Tptr)) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } > + *Tpptr = std::move (*Tpptr); // { dg-warning "moving '\\* Tpptr' of type 'int\\*' to itself" } > + **Tpptr = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } > + Tref = std::move (Tref); // { dg-warning "moving 'Tref' of type 'int' to itself" } > + Trref = std::move (Trref); // { dg-warning "moving 'Trref' of type 'int' to itself" } > + Tcptr = std::move (Tcptr); // { dg-warning "moving 'Tcptr' of type 'const int\\*' to itself" } > + (Tptr) = std::move (Tptr); // { dg-warning "moving 'Tptr' of type 'int\\*' to itself" } > + (*Tptr) = std::move (*Tptr); // { dg-warning "moving '\\* Tptr' of type 'int' to itself" } > + (*Tpptr) = std::move (*Tpptr); // { dg-warning "moving '\\* Tpptr' of type 'int\\*' to itself" } > + (**Tpptr) = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } > + (*(*(Tpptr))) = std::move (**Tpptr); // { dg-warning "moving '\\* \\* Tpptr' of type 'int' to itself" } > + (Tref) = std::move (Tref); // { dg-warning "moving 'Tref' of type 'int' to itself" } > + (Trref) = std::move (Trref); // { dg-warning "moving 'Trref' of type 'int' to itself" } > + (Tcptr) = std::move (Tcptr); // { dg-warning "moving 'Tcptr' of type 'const int\\*' to itself" } > +} > + > +void > +call_testargs () > +{ > + int i = 42; > + int *p = &i; > + testargs<int>(&i, &p, i, 42, &i); > +} > > base-commit: 1e2462890ac748a5e5b6a34fdeb61e7027863a90 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-08-26 18:00 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-09 16:37 [PATCH] c++: Implement -Wself-move warning [PR81159] Marek Polacek 2022-08-15 19:54 ` Jason Merrill 2022-08-18 20:19 ` [PATCH v2] " Marek Polacek 2022-08-19 0:33 ` Jason Merrill 2022-08-19 22:34 ` [PATCH v3] " Marek Polacek 2022-08-20 21:31 ` Jason Merrill 2022-08-23 16:39 ` [PATCH v4] " Marek Polacek 2022-08-23 21:27 ` Jason Merrill 2022-08-24 21:30 ` Marek Polacek 2022-08-25 13:25 ` Jason Merrill 2022-08-25 21:49 ` Marek Polacek 2022-08-26 0:52 ` Jason Merrill 2022-08-26 17:04 ` [PATCH v5] " Marek Polacek 2022-08-26 17:59 ` Jason Merrill
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).