Default std::vector<bool> default and move constructor

[François Dumont]

On 01/06/2017 15:34, Jonathan Wakely wrote:
> On 31/05/17 22:28 +0200, François Dumont wrote:
>> Unless I made a mistake it revealed that restoring explicit call to 
>> _Bit_alloc_type() in default constructor was not enough. G++ doesn't 
>> transform it into a value-init if needed. I don't know if it is a 
>> compiler bug but I had to do just like presented in the Standard to 
>> achieve the expected behavior.
>
> That really shouldn't be necessary (see blow).
>
>> This value-init is specific to post-C++11 right ? Maybe I could 
>> remove the useless explicit call to _Bit_alloc_type() in pre-C++11 
>> mode ?
>
> No, because C++03 also requires the allocator to be value-initialized.

Ok so I'll try to make the test C++03 compatible.

>
>> Now I wonder if I really introduced a regression in rb_tree...
>
> Yes, I think you did. Could you try to verify that using the new
> default_init_allocator?

I did and for the moment I experiment the same result with rb_tree than 
the one I am having with std::vector<bool>, strange.

I plan to add this test to all containers.

>
>
>> +      struct _Bvector_impl
>> +    : public _Bit_alloc_type, public _Bvector_impl_data
>> +    {
>> +    public:
>> +#if __cplusplus >= 201103L
>> +      _Bvector_impl()
>> +        noexcept( noexcept(_Bit_alloc_type())
>> +              && noexcept(_Bvector_impl(declval<const 
>> _Bit_alloc_type&>())) )
>
> This second condition is not needed, because that constructor should
> be noexcept (see below).
>
>> +      : _Bvector_impl(_Bit_alloc_type())
>
> This should not be necessary...
>
>> +      { }
>> +#else
>>       _Bvector_impl()
>> -    : _Bit_alloc_type(), _M_start(), _M_finish(), _M_end_of_storage()
>> +      : _Bit_alloc_type()
>>       { }
>> +#endif
>
> I would expect the constructor to look like this:
>
>       _Bvector_impl()
>       _GLIBCXX_NOEXCEPT_IF( noexcept(_Bit_alloc_type()) )
>          : _Bit_alloc_type()
>          { }
>
> What happens when you do that?

This is what I tried first and test was then failing. It surprised me too.
>
>
>>       _Bvector_impl(const _Bit_alloc_type& __a)
>> -    : _Bit_alloc_type(__a), _M_start(), _M_finish(), 
>> _M_end_of_storage()
>> +         _GLIBCXX_NOEXCEPT_IF( noexcept(_Bit_alloc_type(__a)) )
>
> Copying the allocator is not allowed to throw. You can use simply
> _GLIBCXX_NOEXCEPT here.
>
>
>> +void test01()
>> +{
>> +  typedef default_init_allocator<T> alloc_type;
>> +  typedef std::vector<T, alloc_type> test_type;
>> +
>> +  test_type v1;
>> +  v1.push_back(T());
>> +
>> +  VERIFY( !v1.empty() );
>> +  VERIFY( !v1.get_allocator().state );
>
> This is unlikely to ever fail, because the stack is probably full of
> zeros anyway. Did you confirm whether the test fails without your
> fixes to value-initialize the allocator?
Yes, the test is failing as soon as I use the default constructor just 
calling the allocator default constructor in its initialization list or 
when I default this implementation.
>
> One possible way to make it fail would be to construct the
> vector<bool> using placement new, into a buffer filled with non-zero
> values. (Valgrind or a sanitizer should also tell us, but we can't
> rely on them in the testsuite).
>
This is what I have implemented in this new proposal also considering 
your other remarks. For the moment if the test fail there is a memory 
leak but I prefer to keep implementation simple.

I also start runing the test on the normal std::vector implementation 
and I never managed to make the test fail. Even when I default all 
default constructor implementations !

I started rebuilding everything.

François

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bvector.patch
Type: text/x-patch
Size: 13256 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20170601/7bdf9f62/attachment.bin>