| Message ID | 20240523235711.1301996-1-polacek@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | c++: extend -Wself-move for mem-init-list [PR109396] | expand |
On 5/23/24 19:57, Marek Polacek wrote: > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > -- >8 -- > We already warn for: > > x = std::move (x); > > which triggers: > > warning: moving 'x' of type 'int' to itself [-Wself-move] > > but bug 109396 reports that this doesn't work for a member-initializer-list: > > X() : x(std::move (x)) > > so this patch amends that. > > PR c++/109396 > > gcc/cp/ChangeLog: > > * cp-tree.h (maybe_warn_self_move): Declare. > * init.cc (perform_member_init): Call maybe_warn_self_move. > * typeck.cc (maybe_warn_self_move): No longer static. Change the > return type to bool. Also warn when called from > a member-initializer-list. > > gcc/testsuite/ChangeLog: > > * g++.dg/warn/Wself-move2.C: New test. > --- > gcc/cp/cp-tree.h | 1 + > gcc/cp/init.cc | 5 ++-- > gcc/cp/typeck.cc | 28 +++++++++++++------ > gcc/testsuite/g++.dg/warn/Wself-move2.C | 37 +++++++++++++++++++++++++ > 4 files changed, 60 insertions(+), 11 deletions(-) > create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move2.C > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > index ba9e848c177..ea3fa6f4aac 100644 > --- a/gcc/cp/cp-tree.h > +++ b/gcc/cp/cp-tree.h > @@ -8263,6 +8263,7 @@ extern cp_expr build_c_cast (location_t loc, tree type, > cp_expr expr); > extern tree cp_build_c_cast (location_t, tree, tree, > tsubst_flags_t); > +extern bool maybe_warn_self_move (location_t, tree, tree); > extern cp_expr build_x_modify_expr (location_t, tree, > enum tree_code, tree, > tree, tsubst_flags_t); > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > index 52396d87a8c..4a7ed7f5302 100644 > --- a/gcc/cp/init.cc > +++ b/gcc/cp/init.cc > @@ -999,7 +999,7 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) > if (decl == error_mark_node) > return; > > - if ((warn_init_self || warn_uninitialized) > + if ((warn_init_self || warn_uninitialized || warn_self_move) > && init > && TREE_CODE (init) == TREE_LIST > && TREE_CHAIN (init) == NULL_TREE) > @@ -1013,7 +1013,8 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) > warning_at (DECL_SOURCE_LOCATION (current_function_decl), > OPT_Winit_self, "%qD is initialized with itself", > member); > - else > + else if (!maybe_warn_self_move (input_location, member, > + TREE_VALUE (init))) > find_uninit_fields (&val, &uninitialized, decl); > } > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > index d7fa6e0dd96..e058ce18276 100644 > --- a/gcc/cp/typeck.cc > +++ b/gcc/cp/typeck.cc > @@ -9355,27 +9355,27 @@ cp_build_c_cast (location_t loc, tree type, tree expr, > > /* 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. */ > + assignment. Return true if we warned. */ > > -static void > +bool > maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > { > if (!warn_self_move) > - return; > + return false; > > /* C++98 doesn't know move. */ > if (cxx_dialect < cxx11) > - return; > + return false; > > if (processing_template_decl) > - return; > + return false; > > if (!REFERENCE_REF_P (rhs) > || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > - return; > + return false; > tree fn = TREE_OPERAND (rhs, 0); > if (!is_std_move_p (fn)) > - return; > + return false; > > /* Just a little helper to strip * and various NOPs. */ > auto extract_op = [] (tree &op) { > @@ -9393,13 +9393,23 @@ maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > tree type = TREE_TYPE (lhs); > tree orig_lhs = lhs; > extract_op (lhs); > - if (cp_tree_equal (lhs, arg)) > + if (cp_tree_equal (lhs, arg) > + /* Also warn in a member-initializer-list, as in : i(std::move(i)). */ > + || (TREE_CODE (lhs) == FIELD_DECL > + && TREE_CODE (arg) == COMPONENT_REF > + && cp_tree_equal (TREE_OPERAND (arg, 0), current_class_ref) > + && TREE_OPERAND (arg, 1) == lhs)) > { > 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"); > + { > + inform (loc, "remove %<std::move%> call"); The patch is OK, but why do we suggest removing std::move? That just changes the warning to self-init. Maybe drop the inform? Jason
On Fri, May 24, 2024 at 10:15:56AM -0400, Jason Merrill wrote: > On 5/23/24 19:57, Marek Polacek wrote: > > Bootstrapped/regtested on x86_64-pc-linux-gnu, ok for trunk? > > > > -- >8 -- > > We already warn for: > > > > x = std::move (x); > > > > which triggers: > > > > warning: moving 'x' of type 'int' to itself [-Wself-move] > > > > but bug 109396 reports that this doesn't work for a member-initializer-list: > > > > X() : x(std::move (x)) > > > > so this patch amends that. > > > > PR c++/109396 > > > > gcc/cp/ChangeLog: > > > > * cp-tree.h (maybe_warn_self_move): Declare. > > * init.cc (perform_member_init): Call maybe_warn_self_move. > > * typeck.cc (maybe_warn_self_move): No longer static. Change the > > return type to bool. Also warn when called from > > a member-initializer-list. > > > > gcc/testsuite/ChangeLog: > > > > * g++.dg/warn/Wself-move2.C: New test. > > --- > > gcc/cp/cp-tree.h | 1 + > > gcc/cp/init.cc | 5 ++-- > > gcc/cp/typeck.cc | 28 +++++++++++++------ > > gcc/testsuite/g++.dg/warn/Wself-move2.C | 37 +++++++++++++++++++++++++ > > 4 files changed, 60 insertions(+), 11 deletions(-) > > create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move2.C > > > > diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h > > index ba9e848c177..ea3fa6f4aac 100644 > > --- a/gcc/cp/cp-tree.h > > +++ b/gcc/cp/cp-tree.h > > @@ -8263,6 +8263,7 @@ extern cp_expr build_c_cast (location_t loc, tree type, > > cp_expr expr); > > extern tree cp_build_c_cast (location_t, tree, tree, > > tsubst_flags_t); > > +extern bool maybe_warn_self_move (location_t, tree, tree); > > extern cp_expr build_x_modify_expr (location_t, tree, > > enum tree_code, tree, > > tree, tsubst_flags_t); > > diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc > > index 52396d87a8c..4a7ed7f5302 100644 > > --- a/gcc/cp/init.cc > > +++ b/gcc/cp/init.cc > > @@ -999,7 +999,7 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) > > if (decl == error_mark_node) > > return; > > - if ((warn_init_self || warn_uninitialized) > > + if ((warn_init_self || warn_uninitialized || warn_self_move) > > && init > > && TREE_CODE (init) == TREE_LIST > > && TREE_CHAIN (init) == NULL_TREE) > > @@ -1013,7 +1013,8 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) > > warning_at (DECL_SOURCE_LOCATION (current_function_decl), > > OPT_Winit_self, "%qD is initialized with itself", > > member); > > - else > > + else if (!maybe_warn_self_move (input_location, member, > > + TREE_VALUE (init))) > > find_uninit_fields (&val, &uninitialized, decl); > > } > > diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc > > index d7fa6e0dd96..e058ce18276 100644 > > --- a/gcc/cp/typeck.cc > > +++ b/gcc/cp/typeck.cc > > @@ -9355,27 +9355,27 @@ cp_build_c_cast (location_t loc, tree type, tree expr, > > /* 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. */ > > + assignment. Return true if we warned. */ > > -static void > > +bool > > maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > > { > > if (!warn_self_move) > > - return; > > + return false; > > /* C++98 doesn't know move. */ > > if (cxx_dialect < cxx11) > > - return; > > + return false; > > if (processing_template_decl) > > - return; > > + return false; > > if (!REFERENCE_REF_P (rhs) > > || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) > > - return; > > + return false; > > tree fn = TREE_OPERAND (rhs, 0); > > if (!is_std_move_p (fn)) > > - return; > > + return false; > > /* Just a little helper to strip * and various NOPs. */ > > auto extract_op = [] (tree &op) { > > @@ -9393,13 +9393,23 @@ maybe_warn_self_move (location_t loc, tree lhs, tree rhs) > > tree type = TREE_TYPE (lhs); > > tree orig_lhs = lhs; > > extract_op (lhs); > > - if (cp_tree_equal (lhs, arg)) > > + if (cp_tree_equal (lhs, arg) > > + /* Also warn in a member-initializer-list, as in : i(std::move(i)). */ > > + || (TREE_CODE (lhs) == FIELD_DECL > > + && TREE_CODE (arg) == COMPONENT_REF > > + && cp_tree_equal (TREE_OPERAND (arg, 0), current_class_ref) > > + && TREE_OPERAND (arg, 1) == lhs)) > > { > > 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"); > > + { > > + inform (loc, "remove %<std::move%> call"); > > The patch is OK, but why do we suggest removing std::move? That just > changes the warning to self-init. Maybe drop the inform? Thanks. Yes, I don't know why I added the inform in the first place; there's no point in suggesting changing x = std::move(x); into x = x; I dropped the inform and pushed the following: -- >8 -- We already warn for: x = std::move (x); which triggers: warning: moving 'x' of type 'int' to itself [-Wself-move] but bug 109396 reports that this doesn't work for a member-initializer-list: X() : x(std::move (x)) so this patch amends that. PR c++/109396 gcc/cp/ChangeLog: * cp-tree.h (maybe_warn_self_move): Declare. * init.cc (perform_member_init): Call maybe_warn_self_move. * typeck.cc (maybe_warn_self_move): No longer static. Change the return type to bool. Also warn when called from a member-initializer-list. Drop the inform call. gcc/testsuite/ChangeLog: * g++.dg/warn/Wself-move2.C: New test. --- gcc/cp/cp-tree.h | 1 + gcc/cp/init.cc | 5 ++-- gcc/cp/typeck.cc | 32 +++++++++++---------- gcc/testsuite/g++.dg/warn/Wself-move2.C | 37 +++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 16 deletions(-) create mode 100644 gcc/testsuite/g++.dg/warn/Wself-move2.C diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index 655850a9ab6..6206482c602 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -8263,6 +8263,7 @@ extern cp_expr build_c_cast (location_t loc, tree type, cp_expr expr); extern tree cp_build_c_cast (location_t, tree, tree, tsubst_flags_t); +extern bool maybe_warn_self_move (location_t, tree, tree); extern cp_expr build_x_modify_expr (location_t, tree, enum tree_code, tree, tree, tsubst_flags_t); diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc index 52396d87a8c..4a7ed7f5302 100644 --- a/gcc/cp/init.cc +++ b/gcc/cp/init.cc @@ -999,7 +999,7 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) if (decl == error_mark_node) return; - if ((warn_init_self || warn_uninitialized) + if ((warn_init_self || warn_uninitialized || warn_self_move) && init && TREE_CODE (init) == TREE_LIST && TREE_CHAIN (init) == NULL_TREE) @@ -1013,7 +1013,8 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) warning_at (DECL_SOURCE_LOCATION (current_function_decl), OPT_Winit_self, "%qD is initialized with itself", member); - else + else if (!maybe_warn_self_move (input_location, member, + TREE_VALUE (init))) find_uninit_fields (&val, &uninitialized, decl); } diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index 4a153a8baf9..1b7a31d32f3 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -9355,27 +9355,27 @@ cp_build_c_cast (location_t loc, tree type, tree expr, /* 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. */ + assignment. Return true if we warned. */ -static void +bool maybe_warn_self_move (location_t loc, tree lhs, tree rhs) { if (!warn_self_move) - return; + return false; /* C++98 doesn't know move. */ if (cxx_dialect < cxx11) - return; + return false; if (processing_template_decl) - return; + return false; if (!REFERENCE_REF_P (rhs) || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) - return; + return false; tree fn = TREE_OPERAND (rhs, 0); if (!is_std_move_p (fn)) - return; + return false; /* Just a little helper to strip * and various NOPs. */ auto extract_op = [] (tree &op) { @@ -9393,13 +9393,17 @@ maybe_warn_self_move (location_t loc, tree lhs, tree rhs) 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"); - } + if (cp_tree_equal (lhs, arg) + /* Also warn in a member-initializer-list, as in : i(std::move(i)). */ + || (TREE_CODE (lhs) == FIELD_DECL + && TREE_CODE (arg) == COMPONENT_REF + && cp_tree_equal (TREE_OPERAND (arg, 0), current_class_ref) + && TREE_OPERAND (arg, 1) == lhs)) + if (warning_at (loc, OPT_Wself_move, + "moving %qE of type %qT to itself", orig_lhs, type)) + return true; + + return false; } /* For use from the C common bits. */ diff --git a/gcc/testsuite/g++.dg/warn/Wself-move2.C b/gcc/testsuite/g++.dg/warn/Wself-move2.C new file mode 100644 index 00000000000..0c0e1b9d5f9 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move2.C @@ -0,0 +1,37 @@ +// PR c++/109396 +// { dg-do compile { target c++11 } } +// { dg-options "-Wall" } + +// Define std::move. +namespace std { + template<typename _Tp> + struct remove_reference + { typedef _Tp type; }; + + template<typename _Tp> + struct remove_reference<_Tp&> + { typedef _Tp type; }; + + template<typename _Tp> + struct remove_reference<_Tp&&> + { typedef _Tp type; }; + + template<typename _Tp> + constexpr typename std::remove_reference<_Tp>::type&& + move(_Tp&& __t) noexcept + { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); } +} +struct A { + int i_; + + A(int) : i_(i_) { } // { dg-warning "itself" } + A(int, int) : i_(this->i_) { } // { dg-warning "itself" } +}; + +struct B { + int i_; + + B(int) : i_(std::move(i_)) { } // { dg-warning "itself" } + B(int, int) : i_(std::move(this->i_)) { } // { dg-warning "itself" } +}; + base-commit: 5bc731b83b51910dc7f7cacddb4257a16d62ee38
diff --git a/gcc/cp/cp-tree.h b/gcc/cp/cp-tree.h index ba9e848c177..ea3fa6f4aac 100644 --- a/gcc/cp/cp-tree.h +++ b/gcc/cp/cp-tree.h @@ -8263,6 +8263,7 @@ extern cp_expr build_c_cast (location_t loc, tree type, cp_expr expr); extern tree cp_build_c_cast (location_t, tree, tree, tsubst_flags_t); +extern bool maybe_warn_self_move (location_t, tree, tree); extern cp_expr build_x_modify_expr (location_t, tree, enum tree_code, tree, tree, tsubst_flags_t); diff --git a/gcc/cp/init.cc b/gcc/cp/init.cc index 52396d87a8c..4a7ed7f5302 100644 --- a/gcc/cp/init.cc +++ b/gcc/cp/init.cc @@ -999,7 +999,7 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) if (decl == error_mark_node) return; - if ((warn_init_self || warn_uninitialized) + if ((warn_init_self || warn_uninitialized || warn_self_move) && init && TREE_CODE (init) == TREE_LIST && TREE_CHAIN (init) == NULL_TREE) @@ -1013,7 +1013,8 @@ perform_member_init (tree member, tree init, hash_set<tree> &uninitialized) warning_at (DECL_SOURCE_LOCATION (current_function_decl), OPT_Winit_self, "%qD is initialized with itself", member); - else + else if (!maybe_warn_self_move (input_location, member, + TREE_VALUE (init))) find_uninit_fields (&val, &uninitialized, decl); } diff --git a/gcc/cp/typeck.cc b/gcc/cp/typeck.cc index d7fa6e0dd96..e058ce18276 100644 --- a/gcc/cp/typeck.cc +++ b/gcc/cp/typeck.cc @@ -9355,27 +9355,27 @@ cp_build_c_cast (location_t loc, tree type, tree expr, /* 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. */ + assignment. Return true if we warned. */ -static void +bool maybe_warn_self_move (location_t loc, tree lhs, tree rhs) { if (!warn_self_move) - return; + return false; /* C++98 doesn't know move. */ if (cxx_dialect < cxx11) - return; + return false; if (processing_template_decl) - return; + return false; if (!REFERENCE_REF_P (rhs) || TREE_CODE (TREE_OPERAND (rhs, 0)) != CALL_EXPR) - return; + return false; tree fn = TREE_OPERAND (rhs, 0); if (!is_std_move_p (fn)) - return; + return false; /* Just a little helper to strip * and various NOPs. */ auto extract_op = [] (tree &op) { @@ -9393,13 +9393,23 @@ maybe_warn_self_move (location_t loc, tree lhs, tree rhs) tree type = TREE_TYPE (lhs); tree orig_lhs = lhs; extract_op (lhs); - if (cp_tree_equal (lhs, arg)) + if (cp_tree_equal (lhs, arg) + /* Also warn in a member-initializer-list, as in : i(std::move(i)). */ + || (TREE_CODE (lhs) == FIELD_DECL + && TREE_CODE (arg) == COMPONENT_REF + && cp_tree_equal (TREE_OPERAND (arg, 0), current_class_ref) + && TREE_OPERAND (arg, 1) == lhs)) { 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"); + { + inform (loc, "remove %<std::move%> call"); + return true; + } } + + return false; } /* For use from the C common bits. */ diff --git a/gcc/testsuite/g++.dg/warn/Wself-move2.C b/gcc/testsuite/g++.dg/warn/Wself-move2.C new file mode 100644 index 00000000000..0c0e1b9d5f9 --- /dev/null +++ b/gcc/testsuite/g++.dg/warn/Wself-move2.C @@ -0,0 +1,37 @@ +// PR c++/109396 +// { dg-do compile { target c++11 } } +// { dg-options "-Wall" } + +// Define std::move. +namespace std { + template<typename _Tp> + struct remove_reference + { typedef _Tp type; }; + + template<typename _Tp> + struct remove_reference<_Tp&> + { typedef _Tp type; }; + + template<typename _Tp> + struct remove_reference<_Tp&&> + { typedef _Tp type; }; + + template<typename _Tp> + constexpr typename std::remove_reference<_Tp>::type&& + move(_Tp&& __t) noexcept + { return static_cast<typename std::remove_reference<_Tp>::type&&>(__t); } +} +struct A { + int i_; + + A(int) : i_(i_) { } // { dg-warning "itself" } + A(int, int) : i_(this->i_) { } // { dg-warning "itself" } +}; + +struct B { + int i_; + + B(int) : i_(std::move(i_)) { } // { dg-warning "itself" } + B(int, int) : i_(std::move(this->i_)) { } // { dg-warning "itself" } +}; +