Re: [PATCH] hashmap: adjust documentation to reflect reality

[Jeff King <peff@peff.net>]

On Thu, Nov 30, 2017 at 12:51:41AM +0100, Johannes Schindelin wrote:

> The hashmap API is just complicated enough that even at least one
> long-time Git contributor has to look up how to use it every time he
> finds a new use case. When that happens, it is really useful if the
> provided example code is correct...
> 
> While at it, "fix a memory leak", avoid statements before variable
> declarations, fix a const -> no-const cast, several %l specifiers (which
> want to be %ld), avoid using an undefined constant, call scanf()
> correctly, use FLEX_ALLOC_STR() where appropriate, and adjust the style
> here and there.

Heh, that's quite a list of faults for what's supposed to be simple
example code. ;)

Your improvements all look good to me, and I'd be happy to see this
applied as-is. But here are two possible suggestions:

> diff --git a/hashmap.h b/hashmap.h
> index 7cb29a6aede..7ce79f3f72c 100644
> --- a/hashmap.h
> +++ b/hashmap.h
> @@ -18,75 +18,71 @@
>   *
>   * #define COMPARE_VALUE 1
>   *
> - * static int long2string_cmp(const struct long2string *e1,
> + * static int long2string_cmp(const void *hashmap_cmp_fn_data,
> + *                            const struct long2string *e1,
>   *                            const struct long2string *e2,
> - *                            const void *keydata, const void *userdata)
> + *                            const void *keydata)

If these struct pointers became "const void *", then we would not need
to cast the function pointer here:

>   *     hashmap_init(&map, (hashmap_cmp_fn) long2string_cmp, &flags, 0);

which would mean that the original problem you are fixing would have
been caught by the compiler, rather than probably segfaulting at
runtime.

My second suggestion (which I'm on the fence about) is: would it better
to just say "see t/helper/test-hashmap.c for a representative example?"

I'm all for code examples in documentation, but this one is quite
complex. The code in test-hashmap.c is not much more complex, and is at
least guaranteed to compile and run. It doesn't show off how to combine
a flex-array with a hashmap as well, but I'm not sure how important that
is. So I could go either way.

-Peff