std::vector<bool> code cleanup fixes optimizations

Fri Jul 17 10:36:54 GMT 2020

On 16/12/19 08:18 +0100, FranÃ§ois Dumont wrote:
>A small refresh on this patch now tested also for versioned namespace 
>which require printers.py to be updated.
>
>Note that this simplification works also for normal mode so I can 
>apply it independently from the stl_bvector.h part.
>
>
>Â Â Â  * include/bits/stl_bvector.h
>Â Â Â  [_GLIBCXX_INLINE_VERSION](_Bvector_impl_data::_M_start): Define as
>Â Â Â  _Bit_type*.
>Â Â Â  (_Bvector_impl_data(const _Bvector_impl_data&)): Default.
>Â Â Â  (_Bvector_impl_data(_Bvector_impl_data&&)): Delegate to latter.
>Â Â Â  (_Bvector_impl_data::operator=(const _Bvector_impl_data&)): Default.
>(_Bvector_impl_data::_M_move_data(_Bvector_impl_data&&)): Use latter.
>Â Â Â  (_Bvector_impl_data::_M_reset()): Likewise.
>Â Â Â  (_Bvector_impl_data::_M_swap_data): New.
>Â Â Â  (_Bvector_impl::_Bvector_impl(_Bvector_impl&&)): Implement explicitely.
>Â Â Â  (_Bvector_impl::_Bvector_impl(_Bit_alloc_type&&, 
>_Bvector_impl&&)): New.
>Â Â Â  (_Bvector_base::_Bvector_base(_Bvector_base&&, const allocator_type&)):
>Â Â Â  New, use latter.
>Â Â Â  (vector::vector(vector&&, const allocator_type&, true_type)): New, use
>Â Â Â  latter.
>Â Â Â  (vector::vector(vector&&, const allocator_type&, false_type)): New.
>Â Â Â  (vector::vector(vector&&, const allocator_type&)): Use latters.
>Â Â Â  (vector::vector(const vector&, const allocator_type&)): Adapt.
>Â Â Â  [__cplusplus >= 201103](vector::vector(_InputIt, _InputIt,
>Â Â Â  const allocator_type&)): Use _M_initialize_range.
>Â Â Â  (vector::operator[](size_type)): Use iterator operator[].
>Â Â Â  (vector::operator[](size_type) const): Use const_iterator operator[].
>Â Â Â  (vector::swap(vector&)): Add assertions on allocators. Use 
>_M_swap_data.
>Â Â Â  [__cplusplus >= 201103](vector::insert(const_iterator, _InputIt,
>Â Â Â  _InputIt)): Use _M_insert_range.
>Â Â Â  (vector::_M_initialize(size_type)): Adapt.
>Â Â Â  [__cplusplus >= 201103](vector::_M_initialize_dispatch): Remove.
>Â Â Â  [__cplusplus >= 201103](vector::_M_insert_dispatch): Remove.
>Â Â Â  * python/libstdcxx/v6/printers.py (StdVectorPrinter._iterator): Stop
>Â Â Â  using start _M_offset.
>Â Â Â  (StdVectorPrinter.to_string): Likewise.
>Â Â Â  * testsuite/23_containers/vector/bool/allocator/swap.cc: Adapt.
>Â Â Â  * testsuite/23_containers/vector/bool/cons/noexcept_move_construct.cc:
>Â Â Â  Add check.
>
>FranÃ§ois
>
>
>On 6/24/19 9:31 PM, FranÃ§ois Dumont wrote:
>>Hi
>>
>>Â Â Â  Any feedback regarding this patch ?
>>
>>Thanks
>>
>>On 5/14/19 7:46 AM, FranÃ§ois Dumont wrote:
>>>Hi
>>>
>>>Â Â Â  This is the patch on vector<bool> to:
>>>
>>>- Optimize sizeof in Versioned namespace mode. We could go one 
>>>step further by removing _M_p from _M_finish and just transform it 
>>>into an offset but it is a little bit more impacting for the code.
>>>
>>>- Implement the swap optimization already done on main std::vector 
>>>template class.
>>>
>>>- Fix move constructor so that it is noexcept no matter allocator 
>>>move constructor noexcept qualification
>>>
>>>- Optimize move constructor with allocator when allocator type is 
>>>always equal.
>>>
>>>- Use shortcuts in C++11 by skipping the _M_XXX_dispatch methods. 
>>>Those are now defined only in pre-C++11 mode, I can't see any abi 
>>>issue in doing so.
>>>
>>>Â Â Â  * include/bits/stl_bvector.h
>>>Â Â Â  [_GLIBCXX_INLINE_VERSION](_Bvector_impl_data::_M_start): Define as
>>>Â Â Â  _Bit_type*.
>>>Â Â Â  (_Bvector_impl_data(const _Bvector_impl_data&)): Default.
>>>Â Â Â  (_Bvector_impl_data(_Bvector_impl_data&&)): Delegate to latter.
>>>Â Â Â  (_Bvector_impl_data::operator=(const _Bvector_impl_data&)): Default.
>>>(_Bvector_impl_data::_M_move_data(_Bvector_impl_data&&)): Use latter.
>>>Â Â Â  (_Bvector_impl_data::_M_reset()): Likewise.
>>>Â Â Â  (_Bvector_impl_data::_M_swap_data): New.
>>>Â Â Â  (_Bvector_impl::_Bvector_impl(_Bvector_impl&&)): Implement 
>>>explicitely.
>>>Â Â Â  (_Bvector_impl::_Bvector_impl(_Bit_alloc_type&&, 
>>>_Bvector_impl&&)): New.
>>>Â Â Â  (_Bvector_base::_Bvector_base(_Bvector_base&&, const 
>>>allocator_type&)):
>>>Â Â Â  New, use latter.
>>>Â Â Â  (vector::vector(vector&&, const allocator_type&, true_type)): 
>>>New, use
>>>Â Â Â  latter.
>>>Â Â Â  (vector::vector(vector&&, const allocator_type&, false_type)): New.
>>>Â Â Â  (vector::vector(vector&&, const allocator_type&)): Use latters.
>>>Â Â Â  (vector::vector(const vector&, const allocator_type&)): Adapt.
>>>Â Â Â  [__cplusplus >= 201103](vector::vector(_InputIt, _InputIt,
>>>Â Â Â  const allocator_type&)): Use _M_initialize_range.
>>>Â Â Â  (vector::operator[](size_type)): Use iterator operator[].
>>>Â Â Â  (vector::operator[](size_type) const): Use const_iterator 
>>>operator[].
>>>Â Â Â  (vector::swap(vector&)): Adapt.
>>>Â Â Â  (vector::_M_initialize(size_type)): Add assertions on allocators.
>>>Â Â Â  Use _M_swap_data.
>>>Â Â Â  [__cplusplus >= 201103](vector::insert(const_iterator, _InputIt,
>>>Â Â Â  _InputIt)): Use _M_insert_range.
>>>Â Â Â  [__cplusplus >= 201103](vector::_M_initialize_dispatch): Remove.
>>>Â Â Â  [__cplusplus >= 201103](vector::_M_insert_dispatch): Remove.
>>>Â Â Â  * testsuite/23_containers/vector/bool/allocator/swap.cc: Adapt.
>>>Â Â Â  * 
>>>testsuite/23_containers/vector/bool/cons/noexcept_move_construct.cc:
>>>Â Â Â  Add check.
>>>
>>>Tested under Linux x86_64, normal and debug modes.
>>>
>>>Ok to commit ?

Yes, OK for master, but I have one question below (and one minor
comment).

>diff --git a/libstdc++-v3/include/bits/stl_bvector.h b/libstdc++-v3/include/bits/stl_bvector.h
>index f2eea7799dc..15ecce62683 100644
>--- a/libstdc++-v3/include/bits/stl_bvector.h
>+++ b/libstdc++-v3/include/bits/stl_bvector.h
>@@ -449,7 +449,16 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> 
>       struct _Bvector_impl_data
>       {
>+#if !_GLIBCXX_INLINE_VERSION
> 	_Bit_iterator	_M_start;
>+#else
>+	// We don't need the offset field for the start, it's always zero.
>+	struct {
>+	  _Bit_type* _M_p;
>+	  // Allow assignment from iterators (assume offset is zero):
>+	  void operator=(_Bit_iterator __it) { _M_p = __it._M_p; }
>+	} _M_start;
>+#endif
> 	_Bit_iterator	_M_finish;
> 	_Bit_pointer	_M_end_of_storage;
> 
>@@ -533,6 +555,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> 
> #if __cplusplus >= 201103L
>       _Bvector_base(_Bvector_base&&) = default;
>+
>+      _Bvector_base(_Bvector_base&& __x, const allocator_type& __a)

I think you can add 'noexcept' here.

Constructing the _Bit_alloc_type can't throw, and the base constructor
is noexcept.

>+      : _M_impl(_Bit_alloc_type(__a), std::move(__x._M_impl))
>+      { }
> #endif
> 
>       ~_Bvector_base()

>diff --git a/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/swap.cc b/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/swap.cc
>index de441426532..745fdc85cf6 100644
>--- a/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/swap.cc
>+++ b/libstdc++-v3/testsuite/23_containers/vector/bool/allocator/swap.cc
>@@ -28,19 +28,17 @@ namespace __gnu_test
>   // It is undefined behaviour to swap() containers with unequal allocators
>   // if the allocator doesn't propagate, so ensure the allocators compare
>   // equal, while still being able to test propagation via get_personality().
>+  template<typename Type>
>     bool
>-  operator==(const propagating_allocator<T, false>&,
>-	     const propagating_allocator<T, false>&)
>-  {
>-    return true;
>-  }
>+    operator==(const propagating_allocator<Type, false>&,
>+	       const propagating_allocator<Type, false>&)
>+    { return true; }
> 
>+  template<typename Type>
>     bool
>-  operator!=(const propagating_allocator<T, false>&,
>-	     const propagating_allocator<T, false>&)
>-  {
>-    return false;
>-  }
>+    operator!=(const propagating_allocator<Type, false>&,
>+	       const propagating_allocator<Type, false>&)
>+    { return false; }

Why does this test need to be adapted?