diff mbox

std::vector move assign patch

Message ID 52BDC6AD.5030207@gmail.com
State New
Headers show

Commit Message

François Dumont Dec. 27, 2013, 6:27 p.m. UTC
Hi

     Here is a patch to fix an issue in normal mode during the move 
assignment. The destination vector allocator instance is moved too 
during the assignment which is wrong.

     As I discover this problem while working on issues with management 
of safe iterators during move operations this patch also fix those 
issues in the debug mode for the vector container. Fixes for other 
containers in debug mode will come later.

2013-12-27  François Dumont <fdumont@gcc.gnu.org>

     * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass
     *this allocator instance when building temporary vector instance
     so that *this allocator do not get moved.
     * include/debug/safe_base.h
     (_Safe_sequence_base(_Safe_sequence_base&&)): New.
     * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use
     latter.
     (__gnu_debug::vector<>(vector&&, const allocator_type&)): Swap
     safe iterators if the instance is moved.
     (__gnu_debug::vector<>::operator=(vector&&)): Likewise.
     * testsuite/23_containers/vector/allocator/move.cc (test01): Add
     check on a vector iterator.
     * testsuite/23_containers/vector/allocator/move_assign.cc
     (test02): Likewise.
     (test03): New, test with a non-propagating allocator.
     * testsuite/23_containers/vector/debug/move_assign_neg.cc: New.

Tested under Linux x86_64 normal and debug modes.

I will be in vacation for a week starting today so if you want to apply 
it quickly do not hesitate to do it yourself.

François

Comments

François Dumont Jan. 7, 2014, 9:12 p.m. UTC | #1
I saw on another thread that 4.9 release was getting closer. Applying 
this patch sounds like a good idear for this release.

Any feedback ?


On 12/27/2013 07:27 PM, François Dumont wrote:
> Hi
>
>     Here is a patch to fix an issue in normal mode during the move 
> assignment. The destination vector allocator instance is moved too 
> during the assignment which is wrong.
>
>     As I discover this problem while working on issues with management 
> of safe iterators during move operations this patch also fix those 
> issues in the debug mode for the vector container. Fixes for other 
> containers in debug mode will come later.
>
> 2013-12-27  François Dumont <fdumont@gcc.gnu.org>
>
>     * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass
>     *this allocator instance when building temporary vector instance
>     so that *this allocator do not get moved.
>     * include/debug/safe_base.h
>     (_Safe_sequence_base(_Safe_sequence_base&&)): New.
>     * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use
>     latter.
>     (__gnu_debug::vector<>(vector&&, const allocator_type&)): Swap
>     safe iterators if the instance is moved.
>     (__gnu_debug::vector<>::operator=(vector&&)): Likewise.
>     * testsuite/23_containers/vector/allocator/move.cc (test01): Add
>     check on a vector iterator.
>     * testsuite/23_containers/vector/allocator/move_assign.cc
>     (test02): Likewise.
>     (test03): New, test with a non-propagating allocator.
>     * testsuite/23_containers/vector/debug/move_assign_neg.cc: New.
>
> Tested under Linux x86_64 normal and debug modes.
>
> I will be in vacation for a week starting today so if you want to 
> apply it quickly do not hesitate to do it yourself.
>
> François
>
Jonathan Wakely Jan. 7, 2014, 10:58 p.m. UTC | #2
On 7 January 2014 21:12, François Dumont wrote:
> I saw on another thread that 4.9 release was getting closer. Applying this
> patch sounds like a good idear for this release.
>
> Any feedback ?

Yes, it's coming :-)

I imagine most of us have been on holiday and are just getting back to
speed, and I've also started a new job. I haven't forgotten about the
patch (and if it's appropriate for 4.9 there's still enough time to
get it in).
Jonathan Wakely Jan. 8, 2014, 10:14 a.m. UTC | #3
On 27 December 2013 18:27, François Dumont wrote:
> Hi
>
>     Here is a patch to fix an issue in normal mode during the move
> assignment. The destination vector allocator instance is moved too during
> the assignment which is wrong.

Thanks for your patience, the normal-mode fix is definitely correct,
and I've finished reviewing the other parts and they look good too.

>     As I discover this problem while working on issues with management of
> safe iterators during move operations this patch also fix those issues in
> the debug mode for the vector container. Fixes for other containers in debug
> mode will come later.

OK, great.

In the new test you have:

+  VERIFY( it == v1.begin() ); // Error, it singular

Please change this to "Error, it is singular"

> 2013-12-27  François Dumont <fdumont@gcc.gnu.org>
>
>     * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass
>     *this allocator instance when building temporary vector instance
>     so that *this allocator do not get moved.

Please change this to "does not get moved"


>     * include/debug/safe_base.h
>     (_Safe_sequence_base(_Safe_sequence_base&&)): New.
>     * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use
>     latter.

I don't think "latter" is clear here, please say something like "Use
new move constructor for base class" or "... for _Safe_sequence_base".

This is OK for trunk, thanks very much.

We might also want to fix just the normal-mode part on the 4.8 branch,
I'll think about that.
H.J. Lu Jan. 9, 2014, 12:22 p.m. UTC | #4
On Fri, Dec 27, 2013 at 10:27 AM, François Dumont <frs.dumont@gmail.com> wrote:
> Hi
>
>     Here is a patch to fix an issue in normal mode during the move
> assignment. The destination vector allocator instance is moved too during
> the assignment which is wrong.
>
>     As I discover this problem while working on issues with management of
> safe iterators during move operations this patch also fix those issues in
> the debug mode for the vector container. Fixes for other containers in debug
> mode will come later.
>
> 2013-12-27  François Dumont <fdumont@gcc.gnu.org>
>
>     * include/bits/stl_vector.h (std::vector<>::_M_move_assign): Pass
>     *this allocator instance when building temporary vector instance
>     so that *this allocator do not get moved.
>     * include/debug/safe_base.h
>     (_Safe_sequence_base(_Safe_sequence_base&&)): New.
>     * include/debug/vector (__gnu_debug::vector<>(vector&&)): Use
>     latter.
>     (__gnu_debug::vector<>(vector&&, const allocator_type&)): Swap
>     safe iterators if the instance is moved.
>     (__gnu_debug::vector<>::operator=(vector&&)): Likewise.
>     * testsuite/23_containers/vector/allocator/move.cc (test01): Add
>     check on a vector iterator.
>     * testsuite/23_containers/vector/allocator/move_assign.cc
>     (test02): Likewise.
>     (test03): New, test with a non-propagating allocator.
>     * testsuite/23_containers/vector/debug/move_assign_neg.cc: New.
>
> Tested under Linux x86_64 normal and debug modes.
>
> I will be in vacation for a week starting today so if you want to apply it
> quickly do not hesitate to do it yourself.
>

This caused:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=59738
diff mbox

Patch

Index: include/bits/stl_vector.h
===================================================================
--- include/bits/stl_vector.h	(revision 206214)
+++ include/bits/stl_vector.h	(working copy)
@@ -1433,7 +1433,7 @@ 
       void
       _M_move_assign(vector&& __x, std::true_type) noexcept
       {
-	const vector __tmp(std::move(*this));
+	const vector __tmp(std::move(*this), get_allocator());
 	this->_M_impl._M_swap_data(__x._M_impl);
 	if (_Alloc_traits::_S_propagate_on_move_assign())
 	  std::__alloc_on_move(_M_get_Tp_allocator(),
Index: include/debug/safe_base.h
===================================================================
--- include/debug/safe_base.h	(revision 206214)
+++ include/debug/safe_base.h	(working copy)
@@ -192,6 +192,12 @@ 
     : _M_iterators(0), _M_const_iterators(0), _M_version(1)
     { }
 
+#if __cplusplus >= 201103L
+    _Safe_sequence_base(_Safe_sequence_base&& __x) noexcept
+      : _Safe_sequence_base()
+    { _M_swap(__x); }
+#endif
+
     /** Notify all iterators that reference this sequence that the
 	sequence is being destroyed. */
     ~_Safe_sequence_base()
Index: include/debug/vector
===================================================================
--- include/debug/vector	(revision 206214)
+++ include/debug/vector	(working copy)
@@ -52,6 +52,7 @@ 
       typedef __gnu_debug::_Equal_to<_Base_const_iterator> _Equal;
 
 #if __cplusplus >= 201103L
+      typedef __gnu_debug::_Safe_sequence<vector<_Tp, _Allocator> > _Safe_base;
       typedef __gnu_cxx::__alloc_traits<_Allocator>  _Alloc_traits;
 #endif
 
@@ -111,18 +112,16 @@ 
       vector(const vector& __x)
       : _Base(__x), _M_guaranteed_capacity(__x.size()) { }
 
-      /// Construction from a release-mode vector
+      /// Construction from a normal-mode vector
       vector(const _Base& __x)
       : _Base(__x), _M_guaranteed_capacity(__x.size()) { }
 
 #if __cplusplus >= 201103L
       vector(vector&& __x) noexcept
       : _Base(std::move(__x)),
+	_Safe_base(std::move(__x)),
 	_M_guaranteed_capacity(this->size())
-      {
-	this->_M_swap(__x);
-	__x._M_guaranteed_capacity = 0;
-      }
+      { __x._M_guaranteed_capacity = 0; }
 
       vector(const vector& __x, const allocator_type& __a)
       : _Base(__x, __a), _M_guaranteed_capacity(__x.size()) { }
@@ -131,7 +130,10 @@ 
       : _Base(std::move(__x), __a),
         _M_guaranteed_capacity(this->size())
       {
-	__x._M_invalidate_all();
+	if (__x.get_allocator() == __a)
+	  this->_M_swap(__x);
+	else
+	  __x._M_invalidate_all();
 	__x._M_guaranteed_capacity = 0;
       }
 
@@ -146,7 +148,7 @@ 
       vector&
       operator=(const vector& __x)
       {
-	static_cast<_Base&>(*this) = __x;
+	_M_base() = __x;
 	this->_M_invalidate_all();
 	_M_update_guaranteed_capacity();
 	return *this;
@@ -157,8 +159,13 @@ 
       operator=(vector&& __x) noexcept(_Alloc_traits::_S_nothrow_move())
       {
 	__glibcxx_check_self_move_assign(__x);
-	_Base::operator=(std::move(__x));
-	this->_M_invalidate_all();
+	bool xfer_memory = _Alloc_traits::_S_propagate_on_move_assign()
+	    || __x.get_allocator() == this->get_allocator();
+	_M_base() = std::move(__x._M_base());
+	if (xfer_memory)
+	  this->_M_swap(__x);
+	else
+	  this->_M_invalidate_all();
 	_M_update_guaranteed_capacity();
 	__x._M_invalidate_all();
 	__x._M_guaranteed_capacity = 0;
@@ -168,7 +175,7 @@ 
       vector&
       operator=(initializer_list<value_type> __l)
       {
-	static_cast<_Base&>(*this) = __l;
+	_M_base() = __l;
 	this->_M_invalidate_all();
 	_M_update_guaranteed_capacity();
 	return *this;
Index: testsuite/23_containers/vector/allocator/move.cc
===================================================================
--- testsuite/23_containers/vector/allocator/move.cc	(revision 206214)
+++ testsuite/23_containers/vector/allocator/move.cc	(working copy)
@@ -32,9 +32,11 @@ 
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
   v1 = { T() };
+  auto it = v1.begin();
   test_type v2(std::move(v1));
   VERIFY(1 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
+  VERIFY( it == v2.begin() );
 }
 
 void test02()
Index: testsuite/23_containers/vector/allocator/move_assign.cc
===================================================================
--- testsuite/23_containers/vector/allocator/move_assign.cc	(revision 206214)
+++ testsuite/23_containers/vector/allocator/move_assign.cc	(working copy)
@@ -46,16 +46,35 @@ 
   typedef std::vector<T, alloc_type> test_type;
   test_type v1(alloc_type(1));
   v1.push_back(T());
+  auto it = v1.begin();
   test_type v2(alloc_type(2));
+  v2.push_back(T());
   v2 = std::move(v1);
-  v2.push_back(T());
+  VERIFY( it == v2.begin() );
   VERIFY(0 == v1.get_allocator().get_personality());
   VERIFY(1 == v2.get_allocator().get_personality());
 }
 
+void test03()
+{
+  bool test __attribute__((unused)) = true;
+  typedef propagating_allocator<T, false> alloc_type;
+  typedef std::vector<T, alloc_type> test_type;
+  test_type v1(alloc_type(1));
+  v1.push_back(T());
+  auto it = v1.begin();
+  test_type v2(alloc_type(1));
+  v2.push_back(T());
+  v2 = std::move(v1);
+  VERIFY( it == v2.begin() );
+  VERIFY(1 == v1.get_allocator().get_personality());
+  VERIFY(1 == v2.get_allocator().get_personality());
+}
+
 int main()
 {
   test01();
   test02();
+  test03();
   return 0;
 }
Index: testsuite/23_containers/vector/debug/move_assign_neg.cc
===================================================================
--- testsuite/23_containers/vector/debug/move_assign_neg.cc	(revision 0)
+++ testsuite/23_containers/vector/debug/move_assign_neg.cc	(revision 0)
@@ -0,0 +1,50 @@ 
+// Copyright (C) 2013 Free Software Foundation, Inc.
+//
+// This file is part of the GNU ISO C++ Library.  This library is free
+// software; you can redistribute it and/or modify it under the
+// terms of the GNU General Public License as published by the
+// Free Software Foundation; either version 3, or (at your option)
+// any later version.
+//
+// This library is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License along
+// with this library; see the file COPYING3.  If not see
+// <http://www.gnu.org/licenses/>.
+//
+// { dg-options "-std=gnu++11" }
+// { dg-do run { xfail *-*-* } }
+
+#include <debug/vector>
+
+#include <testsuite_allocator.h>
+#include <testsuite_hooks.h>
+
+void test01()
+{
+  bool test __attribute__((unused)) = true;
+
+  typedef __gnu_test::uneq_allocator<int> alloc_type;
+  typedef __gnu_debug::vector<int, alloc_type> test_type;
+
+  test_type v1(alloc_type(1));
+  v1 = { 0, 1, 2, 3 };
+
+  test_type v2(alloc_type(2));
+  v2 = { 4, 5, 6, 7 };
+
+  auto it = v2.begin();
+
+  v1 = std::move(v2);
+
+  VERIFY( it == v1.begin() ); // Error, it singular
+}
+
+int main()
+{
+  test01();
+  return 0;
+}