On 6/21/19 6:06 AM, Richard Biener wrote: > On Wed, Jun 19, 2019 at 5:15 AM Martin Sebor 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 is actually hash_map, 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?