Re: [PATCH 1/2] hashmap.h: compare function has access to a data field

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 29, 2017, 18:06 UTC
Message-ID: <xmqqvaneu0qy.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170629011334.11173-2-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

> When using the hashmap a common need is to have access to arbitrary data
> in the compare function. A couple of times we abuse the keydata field
> to pass in the data needed. This happens for example in patch-ids.c.

It is not "arbitrary data"; it is very important to streess that we are not just passing random crud, but adding a mechansim to tailor/curry the function in a way that is fixed throughout the lifetime of a hashmap.

I haven't looked at the use of keydata in patch-ids.c and friends to decide if that "abuse" claim is correct; if it were the case, should we expect that a follow-up patch to clean up the existing mess by using the new mechanism? Or does fixing the "abuse" take another mechanism that is different from this one?

Show 7 quoted lines

> While at it improve the naming of the fields of all compare functions used
> by hashmaps by ensuring unused parameters are prefixed with 'unused_' and
> naming the parameters what they are (instead of 'unused' make it
> 'unused_keydata').
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Show 10 quoted lines

> diff --git a/hashmap.h b/hashmap.h
> index de6022a3a9..1c26bbad5b 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -33,11 +33,12 @@ struct hashmap_entry {
>  };
>  
>  typedef int (*hashmap_cmp_fn)(const void *entry, const void *entry_or_key,
> -		const void *keydata);
> +		const void *keydata, const void *cbdata);

As I view the new "data" thing the C's (read: poor-man's) way to cutomize the function, I would have tweaked the function signature by giving the customization data at the front, i.e.

	fn(fndata, entry, entry_or_key, keydata);

as I think the way we wish to be able to express it in a better language would have been something like:

	(partial_apply(fn, fndata))(entry, entry_or_key, keydata)

But what you did is OK, too.

> +extern void hashmap_init(struct hashmap *map,
> +			 hashmap_cmp_fn equals_function,
> +			 const void *data,
> +			 size_t initial_size);

And this does match my expectation ;-).