Re: Remove unordered containers iterators default initialization

[Jonathan Wakely <jwakely dot gcc at gmail dot com>]

On 26 November 2013 21:49, François Dumont wrote:
> On 11/26/2013 12:07 AM, Jonathan Wakely wrote:
>>
>> On 25 November 2013 21:02, François Dumont wrote:
>>>
>>> Hi
>>>
>>>      Following N3644 discussion thread here is a patch proposal to remove
>>> default zero-initialization of unordered containers iterator. I also took
>>> the time to remove default zero-init of nodes _M_nxt pointer.
>>>
>>> 2013-11-25  François Dumont  <fdumont@gcc.gnu.org>
>>>
>>>      * include/bits/hashtable_policy.h (_Hash_node_base): Default
>>>      default constructor.
>>>      (_Node_iterator): Likewise.
>>>      (_Node_const_iterator): Likewise.
>>>      * include/bits/hashtable.h: Adapt.
>>>
>>> Tested under Linux x86_64.
>>>
>>> Ok to commit ?
>>
>> No, I still don't like the idea of leaving data intentionally
>> uninitialized, so I don't like this part of the patch:
>>
>> Index: include/bits/hashtable_policy.h
>> ===================================================================
>> --- include/bits/hashtable_policy.h    (revision 205288)
>> +++ include/bits/hashtable_policy.h    (working copy)
>> @@ -230,7 +230,7 @@
>>     {
>>       _Hash_node_base* _M_nxt;
>>
>> -    _Hash_node_base() noexcept : _M_nxt() { }
>> +    _Hash_node_base() = default;
>>
>>       _Hash_node_base(_Hash_node_base* __next) noexcept : _M_nxt(__next) {
>> }
>>     };
>> @@ -306,6 +306,7 @@
>>
>>         __node_type*  _M_cur;
>>
>> +      _Node_iterator_base() = default;
>>         _Node_iterator_base(__node_type* __p) noexcept
>>         : _M_cur(__p) { }
>>
>>
>> I don't think that will help diagnose errors.
>>
>> Dereferencing a null pointer will usually cause an immediate failure,
>> dereferencing uninitialized pointers can have more subtle, harder to
>> identify problems.
>>
>     Ok, I see, it is not a Standard conforming issue but we prefer to be
> user friendly to performance. I was hoping to make all library iterators
> trivially default constructible and allow some good optimizations (even if
> std::is_trivially_default_constructible is not yet implemented).

IMHO they are not very important optimizations, as I don't think
default-initializing lots of container iterators is common in
performance critical code.

>     Frankly, I think that detecting usage of uninitialized variables is a
> compiler job.

Or separate tools such as valgrind, or as Chris mentioned, the msan
tool, or static analysers.

But valgrind runs very slowly, msan is not part of GCC yet and also
has some performance impact, and reliable static analysers for C++ are
expensive.

> Is it part of the compiler feature or on its TODOs list ?

GCC is notoriously bad at detecting uninitialized variables except in
very trivial cases. Uninitialized members of class objects, for
example, rarely get detected.  Even when that improves, there will be
code that makes it hard for the compiler to detect e.g.

C::iterator
frobnicate(std::vector<C::iterator>& iters)
{
    C::iterator i;   // default-init
    iters.push_back(i);
    return iters[iters.size()/2];
}

I'm in favour of removing redundant initialization where we
immediately re-initialize the memory anyway, but I'm not generally in
favour of removing zero-init so that we hand values garbage to users.