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

From: Jeff King <peff@peff.net>
Date: Nov 30, 2017, 03:07 UTC
Message-ID: <20171130030727.GA24732@sigill.intra.peff.net>
In-Reply-To: <466dd5331907754ca5c25cc83173ca895220ca81.1511999045.git.johannes.schindelin@gmx.de>

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

Show 10 quoted lines

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

Show 14 quoted lines

> 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