Hi Jeff,
On Fri, Apr 6, 2018 at 1:04 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Mon, Apr 02, 2018 at 03:48:50PM -0700, Stefan Beller wrote:
>
>> The diff options are passed to the compare function as
>> 'hashmap_cmp_fn_data', which are given when the hashmaps
>> are initialized.
>>
>> A later patch will make use of the keydata to signal
>> different settings for comparision.
>
> I had to scratch my head here for a moment. Don't we use those options
> as part of the comparison?
Yes we do, but not as passed in here.
Stepping back a bit: There are 2 void pointers passed to the cmp function:
typedef int (*hashmap_cmp_fn)(const void *hashmap_cmp_fn_data,
const void *entry, const void *entry_or_key,
const void *keydata);
The hashmap_cmp_fn_data is the same pointer as we pass as
the equals_function data in
extern void hashmap_init(struct hashmap *map,
hashmap_cmp_fn equals_function,
const void *equals_function_data,
size_t initial_size);
whereas the keydata is passed in either directly when calling cmp directly
or in hashmap_get_from_hash:
static inline void *hashmap_get_from_hash(const struct hashmap *map,
unsigned int hash,
const void *keydata)
{
struct hashmap_entry key;
hashmap_entry_init(&key, hash);
return hashmap_get(map, &key, keydata);
}
It turns out we are passing the struct diff_options *o into the cmp
function twice, once via the inits equals_function_data, as well as
keydata directly. Omit the direct pass in.
This is mostly a cleanup as of now, as it turns out I do not need to
reuse the keydata field anyway.
> I took the "which" to mean "the compare function", but I think you mean
> "we pass these diff options already when the hashmap is initialized".
Oh, yes.
> Maybe something like this would be more clear:
>
> When we initialize the hashmap, we give it a pointer to the
> diff_options, which it then passes along to each call of the
> hashmap_cmp_fn function. There's no need to pass it a second time as
> the "keydata" parameter, and our comparison functions never look at
> keydata.
>
Thanks for clearing this up, will take as-is.
> This was a mistake left over from an earlier round of 2e2d5ac184
> (diff.c: color moved lines differently, 2017-06-30), before hashmap
> learned to pass the data pointer for us.
That sounds about right.
>
> (I'm just guessing on the second paragraph based on a quick look at
> git-blame and my recollection from the time).
>
> -Peff
Thanks,
Stefan
