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).
Richard.
