On Fri, Jun 30, 2017 at 10:34 AM, Junio C Hamano <gits...@pobox.com> wrote:
> Stefan Beller <sbel...@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.
Ah yes, I forgot to fix patch1, when spending all time on the docs in p2.
>
>> 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).
Ok, let me redo the patch to have fndata at the front.
Looking at other places (that have a similar mechanism mechanically,
but are semantically different), such as the callback functions for
the diff machinery, we have the user provided pointer at the end
of the list. But that is because the diff_options are the objects that
should be in front row (they are bound to the callback more than
some caller needed data).
typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
struct diff_options *options, void *data);
typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options
*opt, void *data);
Thanks!
> 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.
