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

[Junio C Hamano]

Stefan Beller  writes:

> Yes it was a last minute squash before sending it out, as the fix was only
> two lines whereas the conversion is a lot. If it were separated I could have
> claimed the introduction to be a rather mechanical patch, but I did not
> make use of coccinelle or such, so the likelihood for errors is just as high.
>
> So I decided to squash them.

I somehow think that logic leads to a suboptimal workflow.  If they
were separated, somebody else could have done an independent
mechanical conversion to verify the result matches yours, which
would give us more confidence.  When such an independent mechanical
conversion does not match, we need one round-trip to ask you if it
was a misconversion or a manual tweak.

In any case, I think I've looked at it long enough to be reasonably
OK with the conversion result myself, so let's move it forward.  Of
course I welcome independent eyeballing by others.

Thanks.

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

[Stefan Beller]

On Thu, Jun 29, 2017 at 11:11 AM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> 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?
>
> I see that you corrected patch-ids.c "while at it".  That may make
> it harder to revert only that "while at it", I suspect.
>
> Thanks.

Yes it was a last minute squash before sending it out, as the fix was only
two lines whereas the conversion is a lot. If it were separated I could have
claimed the introduction to be a rather mechanical patch, but I did not
make use of coccinelle or such, so the likelihood for errors is just as high.

So I decided to squash them.

Thanks,
Stefan

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

[Junio C Hamano]

Junio C Hamano  writes:

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

I see that you corrected patch-ids.c "while at it".  That may make
it harder to revert only that "while at it", I suspect.

Thanks.

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

[Junio C Hamano]

Stefan Beller  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?

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

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