Containers default initialization

[François Dumont]

On 08/06/2017 15:22, Jonathan Wakely wrote:
>
> Oh dear, we have a compiler bug:
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65816

And not a recent one !
>
>> which takes place only if I call this:
>>
>> test_type *tmp = ::new(buf._M_addr()) test_type {};
>>
>> To force default/value init looks like gcc forces you to explicitly 
>> build an allocator instance like in the attached patch.
>
>
> I'm getting uncomfortable with all this churn just to be able to use
> =default, but requiring more new base classes and #if blocks.  We have
> one known regression already, which needs your patch to fix it.
> (Admittedly, it's not a very serious regression, because I think real
> world allocators that are stateful but don't have user-provided
> default constructors are probably very very unlikely).
>
> I'm leaning towards just make the exception specifications correct
> using _GLIBCXX_NOEXCEPT_IF and not doing any refactoring.
>
I prefer this approach because it split responsibilities. I like the one 
class, one responsibility approach. It also avoids repeating the data 
members initialization in several constructors when just a default one 
is needed. And it helps making the move constructor use the default 
implementation.

The purpose of this refactoring was also to bring together the noexcept 
qualification with the operation potentially raising it. It was strange 
to have the noexcept on one constructor but the allocator default 
constructor call at another level.

> That also has the benefit that the code might help guide the future
> direction of the standard, by telling us what conditions could be used
> if we add noexcept to these constructors in the standard.

If it was for doc purpose Doxygen could perhaps help, with gcc help I 
guess. But to fix the regression we need to restore the explicit 
noexcept quaification so we are all happy :-).

>
>
>> Index: include/bits/stl_tree.h
>> ===================================================================
>> --- include/bits/stl_tree.h    (revision 248855)
>> +++ include/bits/stl_tree.h    (working copy)
>> @@ -687,9 +687,17 @@
>>
>> #if __cplusplus < 201103L
>>       _Rb_tree_impl()
>> +      : _Node_allocator()
>>       { }
>> #else
>> -      _Rb_tree_impl() = default;
>> +      _Rb_tree_impl()
>> +        noexcept(
>> +        noexcept(_Node_allocator()) && noexcept(_Base_key_compare()) )
>> +      : _Rb_tree_impl(_Key_compare(), _Node_allocator())
>
> If we didn't have the PR65816 bug this should be simply:
>
>       _Rb_tree_impl()
>       _GLIBCXX_NOEXCEPT_IF(
> is_nothrow_default_constructible<_Key_compare>::value
>           && is_nothrow_default_constructible<_Node_allocator>::value
>       )
>       : _Node_allocator()
>       { }
>
> i.e. the same for C++98 and other modes. Because of the compiler bug
> that doesn't work. I think we should fix the compiler.
>

Yes, that's reassuring, I much prefer this too.

>> +      { }
>> +#endif
>> +
>> +#if __cplusplus >= 201103L
>
> There's no need for an #endif/#if here, the previous block is already
> C++11-and-later code.
>
>> + __gnu_cxx::__aligned_buffer<test_type> buf;
>> +  __builtin_memset(buf._M_addr(), ~0, sizeof(test_type));
>> +
>> +  VERIFY( buf._M_ptr()->get_allocator().state != 0 );
>
> This is undefined. There is no object here, so you can't dereference
> the value returned by bug._M_ptr().

I wanted to make sure that the compiler was not going to optimize the 
memset away as he could. But in -O0 I guess compiler won't do it.

>
>> +  test_type *tmp = ::new(buf._M_addr()) test_type();
>
> This file is called default_init.cc but this is value-initialization.
>

Oh, I was seeing things the wrong way so.

> Can't we just have one file per container type (maybe just called
> default_init.cc) which tests default-initialization in test01() and
> value-initialization in a test02() function?
>
While working on this weird behavior I wanted to always test the 2 types 
of init. If they were in the same file the 1st failure would stop the 
tests. Do you really want them in the same file ?

But is there any dg-xxx flag to put to indicate a failing test for known 
reason ? Or should I simply keep the test on my desktop waiting for the 
compiler fix ?

Attach is the new patch tested under Linux x86_64 normal mode. Note that 
I added a simplification of the _M_get_Node_allocator(), static_cast is 
useless no ?

Ok to commit ?

François

-------------- next part --------------
A non-text attachment was scrubbed...
Name: stl_tree.h.patch
Type: text/x-patch
Size: 8507 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/libstdc++/attachments/20170610/3118d9fc/attachment.bin>