[pulseaudio-discuss] [PATCH] hashmap: Add the ability to free keys

[Arun Raghavan]

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.

Cheers,
Arun