{"thread":{"id":"47350","subject":"[PATCH] hashmap: adjust documentation to reflect reality","startedAt":"2017-11-29T23:52:15Z","lastAt":"2017-12-08T09:03:56Z","messageCount":7,"participants":["Johannes Schindelin","Jonathan Nieder","Jeff King","Junio C Hamano","Stefan Beller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"333820","messageId":"466dd5331907754ca5c25cc83173ca895220ca81.1511999045.git.johannes.schindelin@gmx.de","threadId":"47350","inReplyTo":null,"subject":"[PATCH] hashmap: adjust documentation to reflect reality","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-29T23:51:41Z","receivedAt":"2017-11-29T23:52:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The hashmap API is just complicated enough that even at least one\nlong-time Git contributor has to look up how to use it every time he\nfinds a new use case. When that happens, it is really useful if the\nprovided example code is correct...\n\nWhile at it, \"fix a memory leak\", avoid statements before variable\ndeclarations, fix a const -> no-const cast, several %l specifiers (which\nwant to be %ld), avoid using an undefined constant, call scanf()\ncorrectly, use FLEX_ALLOC_STR() where appropriate, and adjust the style\nhere and there.\n\nSigned-off-by: Johannes Schindelin \n---\n hashmap.h | 60 +++++++++++++++++++++++++++++-------------------------------\n 1 file changed, 29 insertions(+), 31 deletions(-)\n\ndiff --git a/hashmap.h b/hashmap.h\nindex 7cb29a6aede..7ce79f3f72c 100644\n--- a/hashmap.h\n+++ b/hashmap.h\n@@ -18,75 +18,71 @@\n *\n * #define COMPARE_VALUE 1\n *\n- * static int long2string_cmp(const struct long2string *e1,\n+ * static int long2string_cmp(const void *hashmap_cmp_fn_data,\n+ * const struct long2string *e1,\n * const struct long2string *e2,\n- * const void *keydata, const void *userdata)\n+ * const void *keydata)\n * {\n- * char *string = keydata;\n- * unsigned *flags = (unsigned*)userdata;\n+ * const char *string = keydata;\n+ * unsigned flags = *(unsigned *)hashmap_cmp_fn_data;\n *\n * if (flags & COMPARE_VALUE)\n- * return !(e1->key == e2->key) || (keydata ?\n- * strcmp(e1->value, keydata) : strcmp(e1->value, e2->value));\n+ * return e1->key != e2->key ||\n+ * strcmp(e1->value, string ? string : e2->value);\n * else\n- * return !(e1->key == e2->key);\n+ * return e1->key != e2->key;\n * }\n *\n * int main(int argc, char **argv)\n * {\n * long key;\n- * char *value, *action;\n- *\n- * unsigned flags = ALLOW_DUPLICATE_KEYS;\n+ * char value[255], action[32];\n+ * unsigned flags = 0;\n *\n * hashmap_init(&map, (hashmap_cmp_fn) long2string_cmp, &flags, 0);\n *\n- * while (scanf(\"%s %l %s\", action, key, value)) {\n+ * while (scanf(\"%s %ld %s\", action, &key, value)) {\n *\n * if (!strcmp(\"add\", action)) {\n * struct long2string *e;\n- * e = malloc(sizeof(struct long2string) + strlen(value));\n+ * FLEX_ALLOC_STR(e, value, value);\n * hashmap_entry_init(e, memhash(&key, sizeof(long)));\n * e->key = key;\n- * memcpy(e->value, value, strlen(value));\n * hashmap_add(&map, e);\n * }\n *\n * if (!strcmp(\"print_all_by_key\", action)) {\n- * flags &= ~COMPARE_VALUE;\n- *\n- * struct long2string k;\n+ * struct long2string k, *e;\n * hashmap_entry_init(&k, memhash(&key, sizeof(long)));\n * k.key = key;\n *\n- * struct long2string *e = hashmap_get(&map, &k, NULL);\n+ * flags &= ~COMPARE_VALUE;\n+ * e = hashmap_get(&map, &k, NULL);\n * if (e) {\n- * printf(\"first: %l %s\\n\", e->key, e->value);\n- * while (e = hashmap_get_next(&map, e))\n- * printf(\"found more: %l %s\\n\", e->key, e->value);\n+ * printf(\"first: %ld %s\\n\", e->key, e->value);\n+ * while ((e = hashmap_get_next(&map, e)))\n+ * printf(\"found more: %ld %s\\n\", e->key, e->value);\n * }\n * }\n *\n * if (!strcmp(\"has_exact_match\", action)) {\n- * flags |= COMPARE_VALUE;\n- *\n * struct long2string *e;\n- * e = malloc(sizeof(struct long2string) + strlen(value));\n+ * FLEX_ALLOC_STR(e, value, value);\n * hashmap_entry_init(e, memhash(&key, sizeof(long)));\n * e->key = key;\n- * memcpy(e->value, value, strlen(value));\n *\n- * printf(\"%s found\\n\", hashmap_get(&map, e, NULL) ? \"\" : \"not\");\n+ * flags |= COMPARE_VALUE;\n+ * printf(\"%sfound\\n\", hashmap_get(&map, e, NULL) ? \"\" : \"not \");\n+ * free(e);\n * }\n *\n * if (!strcmp(\"has_exact_match_no_heap_alloc\", action)) {\n- * flags |= COMPARE_VALUE;\n- *\n- * struct long2string e;\n- * hashmap_entry_init(e, memhash(&key, sizeof(long)));\n- * e.key = key;\n+ * struct long2string k;\n+ * hashmap_entry_init(&k, memhash(&key, sizeof(long)));\n+ * k.key = key;\n *\n- * printf(\"%s found\\n\", hashmap_get(&map, e, value) ? \"\" : \"not\");\n+ * flags |= COMPARE_VALUE;\n+ * printf(\"%sfound\\n\", hashmap_get(&map, &k, value) ? \"\" : \"not \");\n * }\n *\n * if (!strcmp(\"end\", action)) {\n@@ -94,6 +90,8 @@\n * break;\n * }\n * }\n+ *\n+ * return 0;\n * }\n */\n \n\nbase-commit: 1a4e40aa5dc16564af879142ba9dfbbb88d1e5ff\n-- \n2.15.0.windows.1.22.g2b9dc9b294f\n\nPublished-As: https://github.com/dscho/git/releases/tag/fix-hashmap-documentation-v1\nFetch-It-Via: git fetch https://github.com/dscho/git fix-hashmap-documentation-v1\n"},{"id":"333822","messageId":"20171130001612.GE15098@aiede.mtv.corp.google.com","threadId":"47350","inReplyTo":"466dd5331907754ca5c25cc83173ca895220ca81.1511999045.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] hashmap: adjust documentation to reflect reality","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-11-30T00:16:12Z","receivedAt":"2017-11-30T00:16:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johannes Schindelin wrote:\n\n> The hashmap API is just complicated enough that even at least one\n> long-time Git contributor has to look up how to use it every time he\n> finds a new use case. When that happens, it is really useful if the\n> provided example code is correct...\n>\n> While at it, \"fix a memory leak\", avoid statements before variable\n> declarations, fix a const -> no-const cast, several %l specifiers (which\n> want to be %ld), avoid using an undefined constant, call scanf()\n> correctly, use FLEX_ALLOC_STR() where appropriate, and adjust the style\n> here and there.\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> hashmap.h | 60 +++++++++++++++++++++++++++++-------------------------------\n> 1 file changed, 29 insertions(+), 31 deletions(-)\n\nYay, thanks for this.\n\nReviewed-by: Jonathan Nieder \n\nFollow-on idea for the interested: would making a test that extracts\nthis sample code from hashmap.h and confirms it still works be a crazy\nidea?\n"},{"id":"333831","messageId":"20171130030727.GA24732@sigill.intra.peff.net","threadId":"47350","inReplyTo":"466dd5331907754ca5c25cc83173ca895220ca81.1511999045.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] hashmap: adjust documentation to reflect reality","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-30T03:07:28Z","receivedAt":"2017-11-30T03:07:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 30, 2017 at 12:51:41AM +0100, Johannes Schindelin wrote:\n\n> The hashmap API is just complicated enough that even at least one\n> long-time Git contributor has to look up how to use it every time he\n> finds a new use case. When that happens, it is really useful if the\n> provided example code is correct...\n> \n> While at it, \"fix a memory leak\", avoid statements before variable\n> declarations, fix a const -> no-const cast, several %l specifiers (which\n> want to be %ld), avoid using an undefined constant, call scanf()\n> correctly, use FLEX_ALLOC_STR() where appropriate, and adjust the style\n> here and there.\n\nHeh, that's quite a list of faults for what's supposed to be simple\nexample code. ;)\n\nYour improvements all look good to me, and I'd be happy to see this\napplied as-is. But here are two possible suggestions:\n\n> diff --git a/hashmap.h b/hashmap.h\n> index 7cb29a6aede..7ce79f3f72c 100644\n> --- a/hashmap.h\n> +++ b/hashmap.h\n> @@ -18,75 +18,71 @@\n> *\n> * #define COMPARE_VALUE 1\n> *\n> - * static int long2string_cmp(const struct long2string *e1,\n> + * static int long2string_cmp(const void *hashmap_cmp_fn_data,\n> + * const struct long2string *e1,\n> * const struct long2string *e2,\n> - * const void *keydata, const void *userdata)\n> + * const void *keydata)\n\nIf these struct pointers became \"const void *\", then we would not need\nto cast the function pointer here:\n\n> * hashmap_init(&map, (hashmap_cmp_fn) long2string_cmp, &flags, 0);\n\nwhich would mean that the original problem you are fixing would have\nbeen caught by the compiler, rather than probably segfaulting at\nruntime.\n\nMy second suggestion (which I'm on the fence about) is: would it better\nto just say \"see t/helper/test-hashmap.c for a representative example?\"\n\nI'm all for code examples in documentation, but this one is quite\ncomplex. The code in test-hashmap.c is not much more complex, and is at\nleast guaranteed to compile and run. It doesn't show off how to combine\na flex-array with a hashmap as well, but I'm not sure how important that\nis. So I could go either way.\n\n-Peff\n"},{"id":"333986","messageId":"xmqqindojrw1.fsf@gitster.mtv.corp.google.com","threadId":"47350","inReplyTo":"20171130030727.GA24732@sigill.intra.peff.net","subject":"Re: [PATCH] hashmap: adjust documentation to reflect reality","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-03T05:35:10Z","receivedAt":"2017-12-03T05:35:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My second suggestion (which I'm on the fence about) is: would it better\n> to just say \"see t/helper/test-hashmap.c for a representative example?\"\n\nI also had the same thought. It is rather unwieldy to ask people to\nlift code from comment text, and it is also hard to maintain such a\ncommented out code to make sure it is up to date.\n\n> I'm all for code examples in documentation, but this one is quite\n> complex. The code in test-hashmap.c is not much more complex, and is at\n> least guaranteed to compile and run.\n\nYup. Exactly.\n\n> It doesn't show off how to combine a flex-array with a hashmap as\n> well, but I'm not sure how important that is. So I could go either\n> way.\n\nLikewise.\n\nIn any case, keeping a bad example as-is is less good than replacing\nit with a corrected one, so I do not mind taking this patch as an\nimmediate first step, whether we later decide to remove it and refer\nto an external file that has a real example that will be easier to\nmaintain and use.\n\nThanks.\n"},{"id":"334078","messageId":"CAGZ79kbDevUjjAzvfa-Un6=fnFZu+kLx7bF3vuheXZdxJWvaRw@mail.gmail.com","threadId":"47350","inReplyTo":"xmqqindojrw1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] hashmap: adjust documentation to reflect reality","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-12-04T19:08:35Z","receivedAt":"2017-12-04T19:08:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Dec 2, 2017 at 9:35 PM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> My second suggestion (which I'm on the fence about) is: would it better\n>> to just say \"see t/helper/test-hashmap.c for a representative example?\"\n\nI think that may be better in the long run, indeed.\n\n>\n> I also had the same thought. It is rather unwieldy to ask people to\n> lift code from comment text, and it is also hard to maintain such a\n> commented out code to make sure it is up to date.\n>\n>> I'm all for code examples in documentation, but this one is quite\n>> complex. The code in test-hashmap.c is not much more complex, and is at\n>> least guaranteed to compile and run.\n>\n> Yup. Exactly.\n>\n>> It doesn't show off how to combine a flex-array with a hashmap as\n>> well, but I'm not sure how important that is. So I could go either\n>> way.\n\nWe could add that example to the test helper as then we have a good (tested)\nexample for that case, too.\n\n> In any case, keeping a bad example as-is is less good than replacing\n> it with a corrected one, so I do not mind taking this patch as an\n> immediate first step, whether we later decide to remove it and refer\n> to an external file that has a real example that will be easier to\n> maintain and use.\n>\n> Thanks.\n\nThanks for taking this and building on top,\nStefan\n"},{"id":"334381","messageId":"alpine.DEB.2.21.1.1712072244040.4318@virtualbox","threadId":"47350","inReplyTo":"CAGZ79kbDevUjjAzvfa-Un6=fnFZu+kLx7bF3vuheXZdxJWvaRw@mail.gmail.com","subject":"Re: [PATCH] hashmap: adjust documentation to reflect reality","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-12-07T21:47:43Z","receivedAt":"2017-12-07T21:48:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 4 Dec 2017, Stefan Beller wrote:\n\n> On Sat, Dec 2, 2017 at 9:35 PM, Junio C Hamano wrote:\n> > Jeff King writes:\n> >\n> >> My second suggestion (which I'm on the fence about) is: would it better\n> >> to just say \"see t/helper/test-hashmap.c for a representative example?\"\n> \n> I think that may be better in the long run, indeed.\n\nBut we moved that example already out of the technical API documentation\ninto the hashmap.h file! Too much moving for my taste.\n\n> > I also had the same thought. It is rather unwieldy to ask people to\n> > lift code from comment text, and it is also hard to maintain such a\n> > commented out code to make sure it is up to date.\n> >\n> >> I'm all for code examples in documentation, but this one is quite\n> >> complex. The code in test-hashmap.c is not much more complex, and is at\n> >> least guaranteed to compile and run.\n> >\n> > Yup. Exactly.\n> >\n> >> It doesn't show off how to combine a flex-array with a hashmap as\n> >> well, but I'm not sure how important that is. So I could go either\n> >> way.\n> \n> We could add that example to the test helper as then we have a good (tested)\n> example for that case, too.\n\nWhat we could *also* do, and what would probably make *even more* sense,\nis to simplify the example drastically, to a point where testing it in\ntest-hashmap is pointless, and where a reader can gather *very* quickly\nwhat it takes to use the hashmap routines.\n\nIn any case, I would really like to see my patch applied first, as it is a\nseparate concern from what you gentle people talk about: I simply want\nthat incorrect documentation fixed. The earlier, the better.\n\nCiao,\nDscho\n"},{"id":"334400","messageId":"20171208090346.GB26199@sigill.intra.peff.net","threadId":"47350","inReplyTo":"alpine.DEB.2.21.1.1712072244040.4318@virtualbox","subject":"Re: [PATCH] hashmap: adjust documentation to reflect reality","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T09:03:46Z","receivedAt":"2017-12-08T09:03:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 10:47:43PM +0100, Johannes Schindelin wrote:\n\n> > We could add that example to the test helper as then we have a good (tested)\n> > example for that case, too.\n> \n> What we could *also* do, and what would probably make *even more* sense,\n> is to simplify the example drastically, to a point where testing it in\n> test-hashmap is pointless, and where a reader can gather *very* quickly\n> what it takes to use the hashmap routines.\n\nYes, I'd be in favor of that, too.\n\n> In any case, I would really like to see my patch applied first, as it is a\n> separate concern from what you gentle people talk about: I simply want\n> that incorrect documentation fixed. The earlier, the better.\n\nDefinitely. I think it is in \"next\" already.\n\n-Peff\n"}]}