Re: [PATCH 3/9] hashmap: Call user supplied value free function in insert

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2750 bytes --]

Hi Jukka,

On 02/10/2015 08:42 AM, Jukka Rissanen wrote:
> If user has supplied a value free function, then that will be
> called for each replaced entry.
> ---
>   ell/hashmap.c | 27 ++++++++++++++++++++++++++-
>   1 file changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/ell/hashmap.c b/ell/hashmap.c
> index c51abfd..a204726 100644
> --- a/ell/hashmap.c
> +++ b/ell/hashmap.c
> @@ -393,7 +393,11 @@ LIB_EXPORT void l_hashmap_destroy(struct l_hashmap *hashmap,
>    * @key: key pointer
>    * @value: value pointer
>    *
> - * Insert new @value entry with @key.
> + * Insert new @value entry with @key. If there is already an same key
> + * in the hash, the new value will replace the old one. If user has set
> + * the value free function by l_hashmap_set_value_free_function() then
> + * the user specified free function will be called in order to free the
> + * value.
>    *
>    * Returns: #true when value has been added and #false in case of failure
>    **/
> @@ -419,6 +423,27 @@ LIB_EXPORT bool l_hashmap_insert(struct l_hashmap *hashmap,
>   		goto done;
>   	}
>

So the current implementation allows duplicates to be added.  However, 
it is implementation dependent what happens if you lookup a duplicate 
key.  With the current implementation, the item inserted first is 
returned.  I don't think enforcement of unique keys is required at the 
hashmap_insert level.  The user should not be always made to pay the 
cost of duplicate detection (e.g. if he knows no duplicates will ever be 
present).

If duplicates are an issue, the way to handle this in the client code 
would be to
	- Call l_hashmap_remove first
	- Insert the new element

Alternatively, you could add an l_hashmap_replace() method call, e.g.:

bool l_hashmap_replace(struct l_hashmap *hashmap,
                         const void *key, void *value,
			l_hashmap_destroy_func_t destroy);

> +	/* If the key is already in the hash, we just replace the value */
> +	for (entry = head;; entry = entry->next) {
> +		if (entry->hash == hash &&
> +				!hashmap->compare_func(key, entry->key)) {
> +			if (entry->key != key_new) {
> +				free_key(hashmap, entry->key);
> +				entry->key = key_new;
> +			}

So this part doesn't really match the comment above.  You're also 
freeing the key, which is an unnecessary step.  If the keys are the 
same, just reuse them.

> +
> +			if (entry->value != value) {
> +				free_value(hashmap, entry->value);
> +				entry->value = value;
> +			}
> +
> +			return true;
> +		}
> +
> +		if (entry->next == head)
> +			break;
> +	}
> +
>   	entry = l_new(struct entry, 1);
>   	entry->key = key_new;
>   	entry->value = value;
>

Regards,
-Denis