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
