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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2017, 17:34 UTC
Message-ID: <xmqqlgo9s7li.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170629235336.28460-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 mechanism to tailor/curry the function in a way that is fixed throughout the lifetime of a hashmap.

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);

That would hopefully make it more obvious that the new thing is pairs with fn, not with other arguments (and entry-or-key and keydata pairs, instead of three old arguments standing as equals).

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)

that order would match what is going on better.