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

[David Henningsson]

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?

> 
> Cheers,
> Arun
> 



-- 
David Henningsson, Canonical Ltd.
https://launchpad.net/~diwic