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
