Johannes Schindelin [off-list ref] writes:
Hi Junio,
On Fri, 29 Jul 2016, Junio C Hamano wrote:
quoted
Kevin Willford [off-list ref] writes:
quoted
static int patch_id_cmp(struct patch_id *a,
struct patch_id *b,
- void *keydata)
+ struct diff_options *opt)
{
+ if (is_null_sha1(a->patch_id) &&
+ commit_patch_id(a->commit, opt, a->patch_id, 0))
+ return error("Could not get patch ID for %s",
+ oid_to_hex(&a->commit->object.oid));
+ if (is_null_sha1(b->patch_id) &&
+ commit_patch_id(b->commit, opt, b->patch_id, 0))
+ return error("Could not get patch ID for %s",
+ oid_to_hex(&b->commit->object.oid));
return hashcmp(a->patch_id, b->patch_id);
}
These error returns initially looks slightly iffy in that in general
the caller of any_cmp_fn() wants to know how a/b compares, but by
returning error(), it always says "a is smaller than b".
I am to blame, as this is my design.
And yes, it is kind of funny that we require a cmpfn that returns <0, ==0
and >0 for comparisons, when hashmaps try to avoid any order.
Perhaps hashmap API needs fixing in the longer term not to call this
type hashmap_cmp_fn; instead it should lose cmp and say something
like hashmap_eq_fn or something.
Do you want a note in the commit message about this "abuse" of a negative
return value, or a code comment?
I do not think negative (or non-zero) return is an "abuse" at all.
It is misleading in the context of the function whose name has "cmp"
in it, but that is not the fault of this function, rather, the
breakage is more in the API that calls a function that wants to know
only equality a "cmp". A in-code comment before the function name
may be appropriate:
/*
* hashmap API calls hashmap_cmp_fn, but it only wants
* "does the key match the entry?" with 0 (matches) and
* non-zero (does not match).
*/
static int patch_id_match(const struct patch_id *ent,
const struct patch_id *key,
const void *keydata)
{
...
W dniu 01.08.2016 o 22:11, Junio C Hamano pisze:
Johannes Schindelin [off-list ref] writes:
quoted
On Fri, 29 Jul 2016, Junio C Hamano wrote:
quoted
quoted
These error returns initially looks slightly iffy in that in general
the caller of any_cmp_fn() wants to know how a/b compares, but by
returning error(), it always says "a is smaller than b".
I am to blame, as this is my design.
And yes, it is kind of funny that we require a cmpfn that returns <0, ==0
and >0 for comparisons, when hashmaps try to avoid any order.
Perhaps hashmap API needs fixing in the longer term not to call this
type hashmap_cmp_fn; instead it should lose cmp and say something
like hashmap_eq_fn or something.
The problem is that one expects hashmap_cmp_fn() to return ==0 on equality,
while one would expect for hashmap_eq_fn() to return true (==1) on equality.
So we would have to rewrite all calling sites.
It would be nice to have a comment about how hashmap uses cmpfn in
hashmap.h.
Though... currently our hashmap implementation uses linked
list (separate chaining) for handling hash collisions (for collision
resolution). More sophisticated data structures, such as balanced search
trees, or splay trees, are worth considering only if the load factor is
large, or if the hash distribution is likely to be very non-uniform,
or if one must guarantee good performance even in a worst-case scenario.
So almost certainly comparing function (which is needed for trees)
won't be needed.
Best,
--
Jakub Narębski
Hi Junio,
On Mon, 1 Aug 2016, Junio C Hamano wrote:
Johannes Schindelin [off-list ref] writes:
quoted
On Fri, 29 Jul 2016, Junio C Hamano wrote:
quoted
Kevin Willford [off-list ref] writes:
quoted
static int patch_id_cmp(struct patch_id *a,
struct patch_id *b,
- void *keydata)
+ struct diff_options *opt)
{
+ if (is_null_sha1(a->patch_id) &&
+ commit_patch_id(a->commit, opt, a->patch_id, 0))
+ return error("Could not get patch ID for %s",
+ oid_to_hex(&a->commit->object.oid));
+ if (is_null_sha1(b->patch_id) &&
+ commit_patch_id(b->commit, opt, b->patch_id, 0))
+ return error("Could not get patch ID for %s",
+ oid_to_hex(&b->commit->object.oid));
return hashcmp(a->patch_id, b->patch_id);
}
These error returns initially looks slightly iffy in that in general
the caller of any_cmp_fn() wants to know how a/b compares, but by
returning error(), it always says "a is smaller than b".
I am to blame, as this is my design.
And yes, it is kind of funny that we require a cmpfn that returns <0, ==0
and >0 for comparisons, when hashmaps try to avoid any order.
Perhaps hashmap API needs fixing in the longer term not to call this
type hashmap_cmp_fn; instead it should lose cmp and say something
like hashmap_eq_fn or something.
Maybe.
But to make sure: you do not expect Kevin to do that in the context of
this here patch series, right?
Ciao,
Dscho