std::vector<bool> fix & enhancements

[Jonathan Wakely]

On 30/10/18 07:28 +0100, François Dumont wrote:
>Following Marc Glisse change to ignore _M_start offset I wanted to go 
>a little step further and just remove it in _GLIBCXX_INLINE_VERSION 
>mode.
>
>I also fix a regression we already fixed on mainstream std::vector 
>regarding noexcept qualification of move constructor with allocator.
>
>And I implemented the same optimizations than in std::vector for 
>allocators always comparing equals and for the std::swap operation.
>
>I also avoid re-implementing in vector::operator[] the same code 
>already implemented in iterator::operator[] but this one should 
>perhaps go in a different commit.
>
>
>Â Â Â  * include/bits/stl_bvector.h
>Â Â Â  [_GLIBCXX_INLINE_VERSION](_Bvector_impl_data::_M_start): Define as
>Â Â Â  _Bit_type*.
>Â Â Â  (_Bvector_impl_data(const _Bvector_impl_data&)): New.
>Â Â Â  (_Bvector_impl_data(_Bvector_impl_data&&)): Delegate to latter.
>Â Â Â  (_Bvector_impl_data::operator=(const _Bvector_impl_data&)): New.
>(_Bvector_impl_data::_M_move_data(_Bvector_impl_data&&)): Use latter.
>Â Â Â  (_Bvector_impl_data::_M_reset()): Likewise.
>Â Â Â  (_Bvector_impl_data::_M_begin()): New.
>Â Â Â  (_Bvector_impl_data::_M_cbegin()): New.
>Â Â Â  (_Bvector_impl_data::_M_start_p()): New.
>Â Â Â  (_Bvector_impl_data::_M_set_start(_Bit_type*)): New.
>Â Â Â  (_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.
>Â Â Â  (_Bvector_base::_M_deallocate()): Adapt.
>Â Â Â  (vector::vector(const vector&, const allocator_type&)): Adapt.
>Â Â Â  (vector::vector(vector&&, const allocator_type&, true_type)): New.
>Â Â Â  (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.
>Â Â Â  (vector::begin()): Adapt.
>Â Â Â  (vector::cbegin()): Adapt.
>Â Â Â  (vector::operator[](size_type)): Use iterator operator[].
>Â Â Â  (vector::swap(vector&)): Adapt.
>Â Â Â  (vector::flip()): Adapt.
>Â Â Â  (vector::_M_initialize(size_type)): Adapt.
>Â Â Â  (vector::_M_initialize_value(bool)): Adapt.
>Â Â Â  * include/bits/vector.tcc:
>Â Â Â  (vector<bool>::_M_reallocate(size_type)): Adapt.
>Â Â Â  (vector<bool>::_M_fill_insert(iterator, size_type, bool)): Adapt.
>Â Â Â  (vector<bool>::_M_insert_range<_FwdIter>(iterator, _FwdIter, _FwdIter
>Â Â Â  std::forward_iterator_tag)): Adapt.
>Â Â Â  (vector<bool>::_M_insert_aux(iterator, bool)): Adapt.
>Â Â Â  (std::hash<std::vector<bool>>::operator()): Adapt.
>Â Â Â  * testsuite/23_containers/vector/bool/cons/noexcept_move_construct.cc:
>Â Â Â  Add check.
>
>Tested under Linux x86_64.
>
>Ok to commit ?
>
>François
>

>diff --git a/libstdc++-v3/include/bits/stl_bvector.h b/libstdc++-v3/include/bits/stl_bvector.h
>index 8fbef7a1a3a..81b4a75236d 100644
>--- a/libstdc++-v3/include/bits/stl_bvector.h
>+++ b/libstdc++-v3/include/bits/stl_bvector.h
>@@ -437,7 +437,11 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> 
>       struct _Bvector_impl_data
>       {
>+#if !_GLIBCXX_INLINE_VERSION
> 	_Bit_iterator	_M_start;
>+#else
>+	_Bit_type*	_M_start;
>+#endif

An alternative that would require fewer changes elsewhere in the file
would be:

#if !_GLIBCXX_INLINE_VERSION
 	_Bit_iterator	_M_start;
#else
        // We don't need the offset field for the start pointer,
        // 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

Now the rest of the file doesn't need any checks for
_GLIBCXX_INLINE_VERSION (which I'd prefer to avoid, because it
clutters the code up with extra conditionals for a mode that
**nobody** uses in practice).


> 	_Bit_iterator	_M_finish;
> 	_Bit_pointer	_M_end_of_storage;
> 
>@@ -447,32 +451,74 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER
> 
> #if __cplusplus >= 201103L
> 	_Bvector_impl_data(_Bvector_impl_data&& __x) noexcept
>-	: _M_start(__x._M_start), _M_finish(__x._M_finish)
>-	, _M_end_of_storage(__x._M_end_of_storage)
>+	: _Bvector_impl_data(__x)
> 	{ __x._M_reset(); }
> 
>+	_Bvector_impl_data(const _Bvector_impl_data&) = default;
>+	_Bvector_impl_data&
>+	operator=(const _Bvector_impl_data&) = default;
> 	void
> 	_M_move_data(_Bvector_impl_data&& __x) noexcept
> 	{
>-	  this->_M_start = __x._M_start;
>-	  this->_M_finish = __x._M_finish;
>-	  this->_M_end_of_storage = __x._M_end_of_storage;
>+	  *this = __x;
> 	  __x._M_reset();
> 	}
>+#else
>+	_Bvector_impl_data(const _Bvector_impl_data& __x)
>+	  : _M_start(__x._M_start), _M_finish(__x._M_finish)
>+	  , _M_end_of_storage(__x._M_end_of_storage)
>+	{ }

Do we need this definition? Won't the compiler generate the same thing
for us anyway?

>+	_Bvector_impl_data&
>+	operator=(const _Bvector_impl_data& __x)
>+	{
>+	  _M_start = __x._M_start;
>+	  _M_finish = __x._M_finish;
>+	  _M_end_of_storage = __x._M_end_of_storage;
>+	}

Same here.

>+#endif
>+
>+	_Bit_iterator
>+	_M_begin() const _GLIBCXX_NOEXCEPT
>+	{ return _Bit_iterator(_M_start_p(), 0); }
>+
>+	_Bit_const_iterator
>+	_M_cbegin() const _GLIBCXX_NOEXCEPT
>+	{ return _Bit_const_iterator(_M_start_p(), 0); }
>+
>+	_Bit_type*
>+	_M_start_p() const _GLIBCXX_NOEXCEPT

We already have _M_end_addr() so would _M_begin_addr be a better name
for this new function?

>+#if !_GLIBCXX_INLINE_VERSION
>+	{ return _M_start._M_p; }
>+#else
>+	{ return _M_start; }
>+#endif

Using the suggestion above you wouldn't need #if because return
_M_start._M_p would always be right. But you wouldn't even need this
function at all, the code that uses _M_start._M_p would Just Workâ„¢.


>+	void
>+	_M_set_start(_Bit_type* __p) _GLIBCXX_NOEXCEPT
>+#if !_GLIBCXX_INLINE_VERSION
>+	{ _M_start._M_p = __p; }
>+#else
>+	{ _M_start = __p; }
> #endif

You wouldn't need this function.

etc. etc.


> 
> 	void
> 	_M_reset() _GLIBCXX_NOEXCEPT
>+	{ *this = _Bvector_impl_data(); }
>+
>+	void
>+	_M_swap_data(_Bvector_impl_data& __x) _GLIBCXX_NOEXCEPT
> 	{
>-	  _M_start = _M_finish = _Bit_iterator();
>-	  _M_end_of_storage = _Bit_pointer();
>+	  // Do not use std::swap(_M_start, __x._M_start), etc as it loses
>+	  // information used by TBAA.
>+	  std::swap(*this, __x);
> 	}
>       };
> 
>       struct _Bvector_impl
> 	: public _Bit_alloc_type, public _Bvector_impl_data
>       {
>-	public:
> 	_Bvector_impl()
> 	  _GLIBCXX_NOEXCEPT_IF( noexcept(_Bit_alloc_type()) )

You'll need to rebase the patch, because I just fixed a bug in this
exception specification (r265626).

> 	: _Bit_alloc_type()