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
--
2.45.1
