On Tue, 2013-09-17 at 13:55 +0200, David Henningsson wrote:
> On 09/17/2013 12:34 PM, Arun Raghavan wrote:
> > On Tue, 2013-09-17 at 12:27 +0200, David Henningsson wrote:
> >> On 09/14/2013 08:34 AM, Arun Raghavan wrote:
> >>> Since the hashmap stores a pointer to the key provided at pa_hashmap_put()
> >>> time, it make sense to allow the hashmap to be given ownership of the key and
> >>> have it free it at pa_hashmap_remove/free time.
> >>>
> >>> To do this cleanly, we now provide the key and value free functions at hashmap
> >>> creation time with a pa_hashmap_new_full. With this, we do away with the free
> >>> function that was provided at remove/free time for freeing the value.
> >>> ---
> >>
> >> Since the dbus implementation is now full of typecast from "const char*"
> >> to "char *" (I think?), it would probably make a lot of sense to do
> >> strdup and let the hashmap free the keys, right?
> >
> > Possibly. There's no harm in leaving it as-is, since clearly the
> > lifetime of the string is longer than the hashmap. Maybe Tanu has an
> > opinion on this.
> >
> >> I didn't look it all through, are there other places where you've added
> >> similar typecasts?
> >
> > No, this was the main part.
> >
> > [...]
> >>> static void remove_entry(pa_hashmap *h, struct hashmap_entry *e) {
> >>> pa_assert(h);
> >>> pa_assert(e);
> >>> @@ -94,6 +104,9 @@ static void remove_entry(pa_hashmap *h, struct hashmap_entry *e) {
> >>> BY_HASH(h)[hash] = e->bucket_next;
> >>> }
> >>>
> >>> + if (h->key_free_func)
> >>> + h->key_free_func(e->key);
> >>> +
> >>
> >> So the key_free_func is called on remove_entry, and value_free_func is
> >> called on remove_all. This seems a little counterintuitive at a quick
> >> look. Are the reasons for doing so mostly historical, or is there a
> >> compelling reason to keep it that way?
> >
> > Yes, pa_hashmap_remove() returns the value (it's the equivalent of a
> > get() + remove()), and there are too many callsites to justify the
> > effort of changing these semantics even if we want to.
>
> Hmm. I think we should at least rename one of pa_hashmap_remove and
> pa_hashmap_remove_all then, because they are now different in the way
> they manage objects. Maybe rename pa_hashmap_remove to pa_hashmap_steal,
> since it does almost the same as pa_hashmap_steal_first?
Summarising IRC discussion - I don't think this is a bad idea, but I'm
not volunteering to do it right now, and this shouldn't block the patch
since it's the same behaviour as before.
Cheers,
Arun
