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

[David Henningsson]

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?

I didn't look it all through, are there other places where you've added
similar typecasts?

> diff --git a/src/pulsecore/hashmap.c b/src/pulsecore/hashmap.c
> index 0629164..acac1e0 100644
> --- a/src/pulsecore/hashmap.c
> +++ b/src/pulsecore/hashmap.c
> @@ -35,7 +35,7 @@
>  #define NBUCKETS 127
>  
>  struct hashmap_entry {
> -    const void *key;
> +    void *key;
>      void *value;
>  
>      struct hashmap_entry *bucket_next, *bucket_previous;
> @@ -46,6 +46,9 @@ struct pa_hashmap {
>      pa_hash_func_t hash_func;
>      pa_compare_func_t compare_func;
>  
> +    pa_free_cb_t key_free_func;
> +    pa_free_cb_t value_free_func;
> +
>      struct hashmap_entry *iterate_list_head, *iterate_list_tail;
>      unsigned n_entries;
>  };
> @@ -54,7 +57,7 @@ struct pa_hashmap {
>  
>  PA_STATIC_FLIST_DECLARE(entries, 0, pa_xfree);
>  
> -pa_hashmap *pa_hashmap_new(pa_hash_func_t hash_func, pa_compare_func_t compare_func) {
> +pa_hashmap *pa_hashmap_new_full(pa_hash_func_t hash_func, pa_compare_func_t compare_func, pa_free_cb_t key_free_func, pa_free_cb_t value_free_func) {
>      pa_hashmap *h;
>  
>      h = pa_xmalloc0(PA_ALIGN(sizeof(pa_hashmap)) + NBUCKETS*sizeof(struct hashmap_entry*));
> @@ -62,12 +65,19 @@ pa_hashmap *pa_hashmap_new(pa_hash_func_t hash_func, pa_compare_func_t compare_f
>      h->hash_func = hash_func ? hash_func : pa_idxset_trivial_hash_func;
>      h->compare_func = compare_func ? compare_func : pa_idxset_trivial_compare_func;
>  
> +    h->key_free_func = key_free_func;
> +    h->value_free_func = value_free_func;
> +
>      h->n_entries = 0;
>      h->iterate_list_head = h->iterate_list_tail = NULL;
>  
>      return h;
>  }
>  
> +pa_hashmap *pa_hashmap_new(pa_hash_func_t hash_func, pa_compare_func_t compare_func) {
> +    return pa_hashmap_new_full(hash_func, compare_func, NULL, NULL);
> +}
> +
>  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?

>      if (pa_flist_push(PA_STATIC_FLIST_GET(entries), e) < 0)
>          pa_xfree(e);
>  
> @@ -101,10 +114,10 @@ static void remove_entry(pa_hashmap *h, struct hashmap_entry *e) {
>      h->n_entries--;
>  }
>  
> -void pa_hashmap_free(pa_hashmap *h, pa_free_cb_t free_cb) {
> +void pa_hashmap_free(pa_hashmap *h) {
>      pa_assert(h);
>  
> -    pa_hashmap_remove_all(h, free_cb);
> +    pa_hashmap_remove_all(h);
>      pa_xfree(h);
>  }
>  
> @@ -120,7 +133,7 @@ static struct hashmap_entry *hash_scan(pa_hashmap *h, unsigned hash, const void
>      return NULL;
>  }
>  
> -int pa_hashmap_put(pa_hashmap *h, const void *key, void *value) {
> +int pa_hashmap_put(pa_hashmap *h, void *key, void *value) {
>      struct hashmap_entry *e;
>      unsigned hash;
>  
> @@ -194,7 +207,7 @@ void* pa_hashmap_remove(pa_hashmap *h, const void *key) {
>      return data;
>  }
>  
> -void pa_hashmap_remove_all(pa_hashmap *h, pa_free_cb_t free_cb) {
> +void pa_hashmap_remove_all(pa_hashmap *h) {
>      pa_assert(h);
>  
>      while (h->iterate_list_head) {
> @@ -202,8 +215,8 @@ void pa_hashmap_remove_all(pa_hashmap *h, pa_free_cb_t free_cb) {
>          data = h->iterate_list_head->value;
>          remove_entry(h, h->iterate_list_head);
>  
> -        if (free_cb)
> -            free_cb(data);
> +        if (h->value_free_func)
> +            h->value_free_func(data);
>      }
>  }
>  
> diff --git a/src/pulsecore/hashmap.h b/src/pulsecore/hashmap.h
> index a57fab3..ae030ed 100644
> --- a/src/pulsecore/hashmap.h
> +++ b/src/pulsecore/hashmap.h
> @@ -36,11 +36,15 @@ typedef struct pa_hashmap pa_hashmap;
>  /* Create a new hashmap. Use the specified functions for hashing and comparing objects in the map */
>  pa_hashmap *pa_hashmap_new(pa_hash_func_t hash_func, pa_compare_func_t compare_func);
>  
> -/* Free the hash table. Calls the specified function for every value in the table. The function may be NULL */
> -void pa_hashmap_free(pa_hashmap*, pa_free_cb_t free_cb);
> +/* Create a new hashmap. Use the specified functions for hashing and comparing objects in the map, and functions to free the key
> + * and value (either or both can be NULL). */
> +pa_hashmap *pa_hashmap_new_full(pa_hash_func_t hash_func, pa_compare_func_t compare_func, pa_free_cb_t key_free_func, pa_free_cb_t value_free_func);
> +
> +/* Free the hash table. */
> +void pa_hashmap_free(pa_hashmap*);
>  
>  /* Add an entry to the hashmap. Returns non-zero when the entry already exists */
> -int pa_hashmap_put(pa_hashmap *h, const void *key, void *value);
> +int pa_hashmap_put(pa_hashmap *h, void *key, void *value);
>  
>  /* Return an entry from the hashmap */
>  void* pa_hashmap_get(pa_hashmap *h, const void *key);
> @@ -48,8 +52,8 @@ void* pa_hashmap_get(pa_hashmap *h, const void *key);
>  /* Returns the data of the entry while removing */
>  void* pa_hashmap_remove(pa_hashmap *h, const void *key);
>  
> -/* If free_cb is not NULL, it's called for each entry. */
> -void pa_hashmap_remove_all(pa_hashmap *h, pa_free_cb_t free_cb);
> +/* Remove all entries but don't free the hashmap */
> +void pa_hashmap_remove_all(pa_hashmap *h);
>  
>  /* Return the current number of entries of the hashmap */
>  unsigned pa_hashmap_size(pa_hashmap *h);


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