diff mbox

std::vector move assign patch

Message ID 20170425134050.GF5109@redhat.com
State New
Headers show

Commit Message

Jonathan Wakely April 25, 2017, 1:40 p.m. UTC 
On 25/04/17 13:52 +0100, Jonathan Wakely wrote:
>On 24/04/17 22:10 +0200, Marc Glisse wrote:
>>It seems that this patch had 2 consequences that may or may not have 
>>been planned. Consider this example (from PR64601)
>>
>>#include <vector>
>>typedef std::vector<int> V;
>>void f(V&v,V&w){ V(std::move(w)).swap(v); }
>>void g(V&v,V&w){ v=std::move(w); }
>>
>>1) We generate shorter code for f than for g, probably since the fix 
>>for PR59738. g ends up zeroing v, copying w to v, and finally 
>>zeroing w, and for weird reasons (and because we swap the members 
>>one by one) the standard prevents us from assuming that v and w do 
>>not overlap in weird ways so we cannot optimize as much as one might 
>>expect.
>
>f has an additional precondition (that the allocators of the vectors
>being swapped must propagate on swap or be equal) and so the swap code
>doesn't have to worry about non-equal allocators.
>
>g has to be able to cope with the case where the allocator doesn't
>propagate and isn't equal, and so is more complicated.
>
>However, the propagation trait is known at compile-time, and for the
>common case so is the equality condition, so it's unfortunate if that
>can't be simplified (I'm sure you've analysed it carefully already
>though!)

I tried the attached patch, but it doesn't make any difference
(unsurprisingly, because after inlining we know that:

  if (__rv.get_allocator() != __m)

is false, so making it a compile-time condition doesn't change
anything).

It might be a nice improvement anyway, as it means the
allocator-extended constructor doesn't require movable types if the
allocators are always equal. It extends the set of types that can be
used with that constructor.


>>2) g(v,v) seems to turn v into a nice empty vector,
>
>Yes.
>
>>while f(v,v) turns it into an invalid vector pointing at released 
>>memory.
>
>Does it?! I don't see that happening, and it's a bug if it does.
>
>>Since 2) is a nice side-effect, it may not be worth rewriting 
>>operator= in a way that improves 1) but loses 2). Anyway, just 
>>mentioning this here.
diff mbox

Patch

diff --git a/libstdc++-v3/include/bits/stl_vector.h b/libstdc++-v3/include/bits/stl_vector.h
index fb88212..97c03ac 100644
--- a/libstdc++-v3/include/bits/stl_vector.h
+++ b/libstdc++-v3/include/bits/stl_vector.h
@@ -359,14 +359,8 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
       noexcept(_Alloc_traits::_S_always_equal())
       : _Base(std::move(__rv), __m)
       {
-	if (__rv.get_allocator() != __m)
-	  {
-	    this->_M_impl._M_finish =
-	      std::__uninitialized_move_a(__rv.begin(), __rv.end(),
-					  this->_M_impl._M_start,
-					  _M_get_Tp_allocator());
-	    __rv.clear();
-	  }
+	_M_move_elements(std::move(__rv), __m,
+			 __bool_constant<_Alloc_traits::_S_always_equal()>());
       }
 
       /**
@@ -1522,14 +1516,33 @@  _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
 
 #if __cplusplus >= 201103L
     private:
+      // Used by allocator-extended move constructor for always-equal allocator
+      void
+      _M_move_elements(vector&& __rv, const allocator_type& __m, true_type)
+      { }
+
+      // Used by allocator-extended move constructor for allocators that might
+      // be unequal.
+      void
+      _M_move_elements(vector&& __rv, const allocator_type& __m, false_type)
+      {
+	if (__rv.get_allocator() != __m)
+	  {
+	    this->_M_impl._M_finish =
+	      std::__uninitialized_move_a(__rv.begin(), __rv.end(),
+					  this->_M_impl._M_start,
+					  _M_get_Tp_allocator());
+	    __rv.clear();
+	  }
+      }
+
       // Constant-time move assignment when source object's memory can be
       // moved, either because the source's allocator will move too
       // or because the allocators are equal.
       void
       _M_move_assign(vector&& __x, std::true_type) noexcept
       {
-	vector __tmp(get_allocator());
-	this->_M_impl._M_swap_data(__tmp._M_impl);
+	const vector __tmp(std::move(*this), get_allocator());
 	this->_M_impl._M_swap_data(__x._M_impl);
 	std::__alloc_on_move(_M_get_Tp_allocator(), __x._M_get_Tp_allocator());
       }