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

[Jeff King]

On Thu, Dec 07, 2017 at 10:47:43PM +0100, Johannes Schindelin wrote:

> > We could add that example to the test helper as then we have a good (tested)
> > example for that case, too.
> 
> What we could *also* do, and what would probably make *even more* sense,
> is to simplify the example drastically, to a point where testing it in
> test-hashmap is pointless, and where a reader can gather *very* quickly
> what it takes to use the hashmap routines.

Yes, I'd be in favor of that, too.

> In any case, I would really like to see my patch applied first, as it is a
> separate concern from what you gentle people talk about: I simply want
> that incorrect documentation fixed. The earlier, the better.

Definitely. I think it is in "next" already.

-Peff

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

[Johannes Schindelin]

Hi,

On Mon, 4 Dec 2017, Stefan Beller wrote:

> On Sat, Dec 2, 2017 at 9:35 PM, Junio C Hamano  wrote:
> > Jeff King  writes:
> >
> >> 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 think that may be better in the long run, indeed.

But we moved that example already out of the technical API documentation
into the hashmap.h file! Too much moving for my taste.

> > I also had the same thought.  It is rather unwieldy to ask people to
> > lift code from comment text, and it is also hard to maintain such a
> > commented out code to make sure it is up to date.
> >
> >> 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.
> >
> > Yup.  Exactly.
> >
> >> 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.
> 
> We could add that example to the test helper as then we have a good (tested)
> example for that case, too.

What we could *also* do, and what would probably make *even more* sense,
is to simplify the example drastically, to a point where testing it in
test-hashmap is pointless, and where a reader can gather *very* quickly
what it takes to use the hashmap routines.

In any case, I would really like to see my patch applied first, as it is a
separate concern from what you gentle people talk about: I simply want
that incorrect documentation fixed. The earlier, the better.

Ciao,
Dscho

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

[Stefan Beller]

On Sat, Dec 2, 2017 at 9:35 PM, Junio C Hamano  wrote:
> Jeff King  writes:
>
>> 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 think that may be better in the long run, indeed.

>
> I also had the same thought.  It is rather unwieldy to ask people to
> lift code from comment text, and it is also hard to maintain such a
> commented out code to make sure it is up to date.
>
>> 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.
>
> Yup.  Exactly.
>
>> 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.

We could add that example to the test helper as then we have a good (tested)
example for that case, too.

> In any case, keeping a bad example as-is is less good than replacing
> it with a corrected one, so I do not mind taking this patch as an
> immediate first step, whether we later decide to remove it and refer
> to an external file that has a real example that will be easier to
> maintain and use.
>
> Thanks.

Thanks for taking this and building on top,
Stefan

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

[Junio C Hamano]

Jeff King  writes:

> 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 also had the same thought.  It is rather unwieldy to ask people to
lift code from comment text, and it is also hard to maintain such a
commented out code to make sure it is up to date.

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

Yup.  Exactly.

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

Likewise.

In any case, keeping a bad example as-is is less good than replacing
it with a corrected one, so I do not mind taking this patch as an
immediate first step, whether we later decide to remove it and refer
to an external file that has a real example that will be easier to
maintain and use.

Thanks.

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

[Jeff King]

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(, (hashmap_cmp_fn) long2string_cmp, , 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

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

[Jonathan Nieder]

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.
>
> Signed-off-by: Johannes Schindelin 
> ---
>  hashmap.h | 60 +---
>  1 file changed, 29 insertions(+), 31 deletions(-)

Yay, thanks for this.

Reviewed-by: Jonathan Nieder 

Follow-on idea for the interested: would making a test that extracts
this sample code from hashmap.h and confirms it still works be a crazy
idea?