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

From: Stefan Beller <sbeller@google.com>
Date: Jun 30, 2017, 17:41 UTC
Message-ID: <CAGZ79kbu2hqc41eBk_AtoeTviJGT4TTV7M6cYp0h4q=gJs2wRg@mail.gmail.com>
In-Reply-To: <xmqqlgo9s7li.fsf@gitster.mtv.corp.google.com>

On Fri, Jun 30, 2017 at 10:34 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> 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.

Ah yes, I forgot to fix patch1, when spending all time on the docs in p2.

Show 21 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).

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!

Show 6 quoted lines

> 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.