On 6/24/19 6:11 AM, Richard Biener wrote:
> On Fri, Jun 21, 2019 at 7:17 PM Martin Sebor <msebor@gmail.com> wrote:
>>
>> On 6/21/19 6:06 AM, Richard Biener wrote:
>>> On Wed, Jun 19, 2019 at 5:15 AM Martin Sebor <msebor@gmail.com> wrote:
>>>>
>>>> Bug 90923 shows that even though GCC hash-table based containers
>>>> like hash_map can be instantiated on types with user-defined ctors
>>>> and dtors they invoke the dtors of such types without invoking
>>>> the corresponding ctors.
>>>>
>>>> It was thanks to this bug that I spent a day debugging "interesting"
>>>> miscompilations during GCC bootstrap (in fairness, it was that and
>>>> bug 90904 about auto_vec copy assignment/construction also being
>>>> hosed even for POD types).
>>>>
>>>> The attached patch corrects the hash_map and hash_set templates
>>>> to invoke the ctors of the elements they insert and makes them
>>>> (hopefully) safe to use with non-trivial user-defined types.
>>>
>>> Hum. I am worried about the difference of assignment vs. construction
>>> in ::put()
>>>
>>> + bool ins = hash_entry::is_empty (*e);
>>> + if (ins)
>>> + {
>>> + e->m_key = k;
>>> + new ((void *) &e->m_value) Value (v);
>>> + }
>>> + else
>>> + e->m_value = v;
>>>
>>> why not invoke the dtor on the old value and then the ctor again?
>>
>> It wouldn't work for self-assignment:
>>
>> Value &y = m.get_or_insert (key);
>> m.put (key, y);
>>
>> The usual rule of thumb for writes into containers is to use
>> construction when creating a new element and assignment when
>> replacing the value of an existing element.
>>
>> Which reminds me that the hash containers, despite being copy-
>> constructible (at least for POD types, they aren't for user-
>> defined types), also aren't safe for assignment even for PODs.
>> I opened bug 90959 for this. Until the assignment is fixed
>> I made it inaccessibe in the patch (I have fixed the copy
>> ctor to DTRT for non-PODs).
>>
>>> How is an empty hash_entry constructed?
>>
>> In hash_table::find_slot_with_hash simply by finding an empty
>> slot and returning a pointer to it. The memory for the slot
>> is marked "empty" by calling the Traits::mark_empty() function.
>>
>> The full type of hash_map<void*, Value> is actually
>>
>> hash_map<void*,
>> Value,
>> simple_hashmap_traits<default_hash_traits<void*>,
>> Value>
>>
>> and simple_hashmap_traits delegates it to default_hash_traits
>> whose mark_empty() just clears the void*, leaving the Value
>> part uninitialized. That makes sense because we don't want
>> to call ctors for empty entries. I think the questions one
>> might ask if one were to extend the design are: a) what class
>> should invoke the ctor/assignment and b) should it do it
>> directly or via the traits?
>>
>>> ::remove() doesn't
>>> seem to invoke the dtor either, instead it relies on the
>>> traits::remove function?
>>
>> Yes. There is no Traits::construct or assign or copy. We
>> could add them but I'm not sure I see to what end (there could
>> be use cases, I just don't know enough about these classes to
>> think of any).
>>
>> Attached is an updated patch with the additional minor fixes
>> mentioned above.
>>
>> Martin
>>
>> PS I think we would get much better results by adopting
>> the properly designed and tested standard library containers
>> than by spending time trying to improve the design of these
>> legacy classes. For simple uses that don't need to integrate
>> with the GC machinery the standard containers should be fine
>> (plus, it'd provide us with greater motivation to improve
>> them and the code GCC emits for their uses). Unfortunately,
>> to be able to use the hash-based containers we would need to
>> upgrade to C++ 11. Isn't it time yet?
>
> I don't think so. I'm also not sure if C++11 on its own is desirable
> or if it should be C++14 or later at that point. SLES 12 has GCC 4.8
> as host compiler (but also GCC 7+ optionally), SLES 15 has GCC 7.
> SLES 11 already struggles currently (GCC 4.3) but I'd no longer
> consider that important enough.
>
> Note any such change to libstdc++ containers should be complete
> and come with both code-size and compile-time and memory-usage
> measurements (both of GCC and other apps of course).
Can I go ahead and commit the patch?