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
