On Mon, Jun 24, 2019 at 4:35 PM Martin Sebor <mse...@gmail.com> wrote: > > On 6/24/19 6:11 AM, Richard Biener wrote: > > On Fri, Jun 21, 2019 at 7:17 PM Martin Sebor <mse...@gmail.com> wrote: > >> > >> On 6/21/19 6:06 AM, Richard Biener wrote: > >>> On Wed, Jun 19, 2019 at 5:15 AM Martin Sebor <mse...@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?
I think we need to document the requirements on Value classes better. @@ -177,7 +185,10 @@ public: INSERT); bool ins = Traits::is_empty (*e); if (ins) - e->m_key = k; + { + e->m_key = k; + new ((void *)&e->m_value) Value (); + } this now requires a default constructor and I always forget about differences between the different form of initializations -- for a POD, does this zero the entry? Otherwise looks OK to me - I was hoping Jonathan would chime in here. Richard. > Martin