{"thread":{"id":"47846","subject":"[PATCH 0/6] minor test-hashmap fixes","startedAt":"2018-02-14T18:03:28Z","lastAt":"2018-02-14T19:01:11Z","messageCount":11,"participants":["Jeff King","Stefan Beller","Code AI","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"339295","messageId":"20180214180322.GA9190@sigill.intra.peff.net","threadId":"47846","inReplyTo":null,"subject":"[PATCH 0/6] minor test-hashmap fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:03:22Z","receivedAt":"2018-02-14T18:03:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series started with me fixing the sizeof() mismatch discussed in\n\n  https://public-inbox.org/git/20180214164628.GA907@sigill.intra.peff.net/\n\nbut I found a number of minor irritations. Most of them are cosmetic in\npractice, but I think it's important for test-helper code like this to\nmodel best practices, since people are likely to use it as a reference.\n\n  [1/6]: test-hashmap: use ALLOC_ARRAY rather than bare malloc\n  [2/6]: test-hashmap: check allocation computation for overflow\n  [3/6]: test-hashmap: use xsnprintf rather than snprintf\n  [4/6]: test-hashmap: use strbuf_getline rather than fgets\n  [5/6]: test-hashmap: simplify alloc_test_entry\n  [6/6]: test-hashmap: use \"unsigned int\" for hash storage\n\n t/helper/test-hashmap.c | 53 +++++++++++++++++++++--------------------\n 1 file changed, 27 insertions(+), 26 deletions(-)\n\n-Peff\n"},{"id":"339296","messageId":"20180214180545.GA9919@sigill.intra.peff.net","threadId":"47846","inReplyTo":"20180214180322.GA9190@sigill.intra.peff.net","subject":"[PATCH 1/6] test-hashmap: use ALLOC_ARRAY rather than bare malloc","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:05:46Z","receivedAt":"2018-02-14T18:05:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"These two array allocations have several minor flaws:\n\n  - they use bare malloc, rather than our error-checking\n    xmalloc\n\n  - they do a bare multiplication to determine the total\n    size (which in theory can overflow, though in this case\n    the sizes are all constants)\n\n  - they use sizeof(type), but the type in the second one\n    doesn't match the actual array (though it's \"int\" versus\n    \"unsigned int\", which are guaranteed by C99 to have the\n    same size)\n\nNone of these are likely to be problems in practice, and\nthis is just a test helper. But since people often look at\ntest helpers as reference code, we should do our best to\nmodel the recommended techniques.\n\nSwitching to ALLOC_ARRAY fixes all three.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe sizeof() thing came from Code AI's original email. I'm happy to\ninclude a Reported-by there, but I wasn't sure of the correct entity to\ncredit. :)\n\n t/helper/test-hashmap.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 1145d51671..b36886bf35 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -85,8 +85,8 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n \tunsigned int *hashes;\n \tunsigned int i, j;\n \n-\tentries = malloc(TEST_SIZE * sizeof(struct test_entry *));\n-\thashes = malloc(TEST_SIZE * sizeof(int));\n+\tALLOC_ARRAY(entries, TEST_SIZE);\n+\tALLOC_ARRAY(hashes, TEST_SIZE);\n \tfor (i = 0; i < TEST_SIZE; i++) {\n \t\tsnprintf(buf, sizeof(buf), \"%i\", i);\n \t\tentries[i] = alloc_test_entry(0, buf, strlen(buf), \"\", 0);\n-- \n2.16.1.464.gc4bae515b7\n\n"},{"id":"339297","messageId":"20180214180634.GB9919@sigill.intra.peff.net","threadId":"47846","inReplyTo":"20180214180322.GA9190@sigill.intra.peff.net","subject":"[PATCH 2/6] test-hashmap: check allocation computation for overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:06:34Z","receivedAt":"2018-02-14T18:06:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we allocate the test_entry flex-struct, we have to add\nup all of the elements that go into the flex array. If these\nwere to overflow a size_t, this would allocate a too-small\nbuffer, which we would then overflow in our memcpy steps.\n\nSince this is just a test-helper, it probably doesn't matter\nin practice, but we should model the correct technique by\nusing the st_add() macros.\n\nUnfortunately, we cannot use the FLEX_ALLOC() macros here,\nbecause we are stuffing two different buffers into a single\nflex array.\n\nWhile we're here, let's also swap out \"malloc\" for our\nerror-checking \"xmalloc\", and use the preferred\n\"sizeof(*var)\" instead of \"sizeof(type)\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-hashmap.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex b36886bf35..2100877c2b 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -32,8 +32,7 @@ static int test_entry_cmp(const void *cmp_data,\n static struct test_entry *alloc_test_entry(int hash, char *key, int klen,\n \t\tchar *value, int vlen)\n {\n-\tstruct test_entry *entry = malloc(sizeof(struct test_entry) + klen\n-\t\t\t+ vlen + 2);\n+\tstruct test_entry *entry = xmalloc(st_add4(sizeof(*entry), klen, vlen, 2));\n \thashmap_entry_init(entry, hash);\n \tmemcpy(entry->key, key, klen + 1);\n \tmemcpy(entry->key + klen + 1, value, vlen + 1);\n-- \n2.16.1.464.gc4bae515b7\n\n"},{"id":"339298","messageId":"20180214180656.GC9919@sigill.intra.peff.net","threadId":"47846","inReplyTo":"20180214180322.GA9190@sigill.intra.peff.net","subject":"[PATCH 3/6] test-hashmap: use xsnprintf rather than snprintf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:06:57Z","receivedAt":"2018-02-14T18:07:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In general, using a bare snprintf can truncate the resulting\nbuffer, leading to confusing results. In this case we know\nthat our buffer is sized large enough to accommodate our\nloop, so there's no bug. However, we should use xsnprintf()\nto document (and check) that assumption, and to model good\npractice to people reading the code.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-hashmap.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 2100877c2b..28b913fbd6 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -87,7 +87,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n \tALLOC_ARRAY(entries, TEST_SIZE);\n \tALLOC_ARRAY(hashes, TEST_SIZE);\n \tfor (i = 0; i < TEST_SIZE; i++) {\n-\t\tsnprintf(buf, sizeof(buf), \"%i\", i);\n+\t\txsnprintf(buf, sizeof(buf), \"%i\", i);\n \t\tentries[i] = alloc_test_entry(0, buf, strlen(buf), \"\", 0);\n \t\thashes[i] = hash(method, i, entries[i]->key);\n \t}\n-- \n2.16.1.464.gc4bae515b7\n\n"},{"id":"339299","messageId":"20180214180718.GD9919@sigill.intra.peff.net","threadId":"47846","inReplyTo":"20180214180322.GA9190@sigill.intra.peff.net","subject":"[PATCH 4/6] test-hashmap: use strbuf_getline rather than fgets","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:07:19Z","receivedAt":"2018-02-14T18:07:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Using fgets() with a fixed-size buffer can lead to lines\nbeing accidentally split across two calls if they are larger\nthan the buffer size.\n\nAs this is just a test helper, this is unlikely to be a\nproblem in practice. But since people may look at test\nhelpers as reference code, it's a good idea for them to\nmodel the preferred behavior.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-hashmap.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 28b913fbd6..15fc4e372f 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -1,5 +1,6 @@\n #include \"git-compat-util.h\"\n #include \"hashmap.h\"\n+#include \"strbuf.h\"\n \n struct test_entry\n {\n@@ -143,7 +144,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n  */\n int cmd_main(int argc, const char **argv)\n {\n-\tchar line[1024];\n+\tstruct strbuf line = STRBUF_INIT;\n \tstruct hashmap map;\n \tint icase;\n \n@@ -152,13 +153,13 @@ int cmd_main(int argc, const char **argv)\n \thashmap_init(&map, test_entry_cmp, &icase, 0);\n \n \t/* process commands from stdin */\n-\twhile (fgets(line, sizeof(line), stdin)) {\n+\twhile (strbuf_getline(&line, stdin) != EOF) {\n \t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n \t\tint l1 = 0, l2 = 0, hash = 0;\n \t\tstruct test_entry *entry;\n \n \t\t/* break line into command and up to two parameters */\n-\t\tcmd = strtok(line, DELIM);\n+\t\tcmd = strtok(line.buf, DELIM);\n \t\t/* ignore empty lines */\n \t\tif (!cmd || *cmd == '#')\n \t\t\tcontinue;\n@@ -262,6 +263,7 @@ int cmd_main(int argc, const char **argv)\n \t\t}\n \t}\n \n+\tstrbuf_release(&line);\n \thashmap_free(&map, 1);\n \treturn 0;\n }\n-- \n2.16.1.464.gc4bae515b7\n\n"},{"id":"339301","messageId":"20180214180819.GE9919@sigill.intra.peff.net","threadId":"47846","inReplyTo":"20180214180322.GA9190@sigill.intra.peff.net","subject":"[PATCH 5/6] test-hashmap: simplify alloc_test_entry","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:08:20Z","receivedAt":"2018-02-14T18:08:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This function takes two ptr/len pairs, which implies that\nthey can be arbitrary buffers. But internally, it assumes\nthat each \"ptr\" is NUL-terminated at \"len\" (because we\nmemcpy an extra byte to pick up the NUL terminator).\n\nIn practice this works because each caller only ever passes\nstrlen(ptr) as the length. But let's drop the \"len\"\nparameters to make our expectations clear.\n\nNote that we can get rid of the \"l1\" and \"l2\" variables from\ncmd_main() as a further cleanup, since they are now mostly\nused to check whether the p1 and p2 arguments are present\n(technically the length parameters conflated NULL with the\nempty string, which we no longer do, but I think that is\nactually an improvement).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-hashmap.c | 35 +++++++++++++++++------------------\n 1 file changed, 17 insertions(+), 18 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 15fc4e372f..56efff36e8 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -30,9 +30,10 @@ static int test_entry_cmp(const void *cmp_data,\n \t\treturn strcmp(e1->key, key ? key : e2->key);\n }\n \n-static struct test_entry *alloc_test_entry(int hash, char *key, int klen,\n-\t\tchar *value, int vlen)\n+static struct test_entry *alloc_test_entry(int hash, char *key, char *value)\n {\n+\tsize_t klen = strlen(key);\n+\tsize_t vlen = strlen(value);\n \tstruct test_entry *entry = xmalloc(st_add4(sizeof(*entry), klen, vlen, 2));\n \thashmap_entry_init(entry, hash);\n \tmemcpy(entry->key, key, klen + 1);\n@@ -89,7 +90,7 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n \tALLOC_ARRAY(hashes, TEST_SIZE);\n \tfor (i = 0; i < TEST_SIZE; i++) {\n \t\txsnprintf(buf, sizeof(buf), \"%i\", i);\n-\t\tentries[i] = alloc_test_entry(0, buf, strlen(buf), \"\", 0);\n+\t\tentries[i] = alloc_test_entry(0, buf, \"\");\n \t\thashes[i] = hash(method, i, entries[i]->key);\n \t}\n \n@@ -155,7 +156,7 @@ int cmd_main(int argc, const char **argv)\n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n \t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n-\t\tint l1 = 0, l2 = 0, hash = 0;\n+\t\tint hash = 0;\n \t\tstruct test_entry *entry;\n \n \t\t/* break line into command and up to two parameters */\n@@ -166,31 +167,29 @@ int cmd_main(int argc, const char **argv)\n \n \t\tp1 = strtok(NULL, DELIM);\n \t\tif (p1) {\n-\t\t\tl1 = strlen(p1);\n \t\t\thash = icase ? strihash(p1) : strhash(p1);\n \t\t\tp2 = strtok(NULL, DELIM);\n-\t\t\tif (p2)\n-\t\t\t\tl2 = strlen(p2);\n \t\t}\n \n-\t\tif (!strcmp(\"hash\", cmd) && l1) {\n+\t\tif (!strcmp(\"hash\", cmd) && p1) {\n \n \t\t\t/* print results of different hash functions */\n-\t\t\tprintf(\"%u %u %u %u\\n\", strhash(p1), memhash(p1, l1),\n-\t\t\t\t\tstrihash(p1), memihash(p1, l1));\n+\t\t\tprintf(\"%u %u %u %u\\n\",\n+\t\t\t       strhash(p1), memhash(p1, strlen(p1)),\n+\t\t\t       strihash(p1), memihash(p1, strlen(p1)));\n \n-\t\t} else if (!strcmp(\"add\", cmd) && l1 && l2) {\n+\t\t} else if (!strcmp(\"add\", cmd) && p1 && p2) {\n \n \t\t\t/* create entry with key = p1, value = p2 */\n-\t\t\tentry = alloc_test_entry(hash, p1, l1, p2, l2);\n+\t\t\tentry = alloc_test_entry(hash, p1, p2);\n \n \t\t\t/* add to hashmap */\n \t\t\thashmap_add(&map, entry);\n \n-\t\t} else if (!strcmp(\"put\", cmd) && l1 && l2) {\n+\t\t} else if (!strcmp(\"put\", cmd) && p1 && p2) {\n \n \t\t\t/* create entry with key = p1, value = p2 */\n-\t\t\tentry = alloc_test_entry(hash, p1, l1, p2, l2);\n+\t\t\tentry = alloc_test_entry(hash, p1, p2);\n \n \t\t\t/* add / replace entry */\n \t\t\tentry = hashmap_put(&map, entry);\n@@ -199,7 +198,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\tputs(entry ? get_value(entry) : \"NULL\");\n \t\t\tfree(entry);\n \n-\t\t} else if (!strcmp(\"get\", cmd) && l1) {\n+\t\t} else if (!strcmp(\"get\", cmd) && p1) {\n \n \t\t\t/* lookup entry in hashmap */\n \t\t\tentry = hashmap_get_from_hash(&map, hash, p1);\n@@ -212,7 +211,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\t\tentry = hashmap_get_next(&map, entry);\n \t\t\t}\n \n-\t\t} else if (!strcmp(\"remove\", cmd) && l1) {\n+\t\t} else if (!strcmp(\"remove\", cmd) && p1) {\n \n \t\t\t/* setup static key */\n \t\t\tstruct hashmap_entry key;\n@@ -238,7 +237,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\tprintf(\"%u %u\\n\", map.tablesize,\n \t\t\t       hashmap_get_size(&map));\n \n-\t\t} else if (!strcmp(\"intern\", cmd) && l1) {\n+\t\t} else if (!strcmp(\"intern\", cmd) && p1) {\n \n \t\t\t/* test that strintern works */\n \t\t\tconst char *i1 = strintern(p1);\n@@ -252,7 +251,7 @@ int cmd_main(int argc, const char **argv)\n \t\t\telse\n \t\t\t\tprintf(\"%s\\n\", i1);\n \n-\t\t} else if (!strcmp(\"perfhashmap\", cmd) && l1 && l2) {\n+\t\t} else if (!strcmp(\"perfhashmap\", cmd) && p1 && p2) {\n \n \t\t\tperf_hashmap(atoi(p1), atoi(p2));\n \n-- \n2.16.1.464.gc4bae515b7\n\n"},{"id":"339303","messageId":"20180214180857.GF9919@sigill.intra.peff.net","threadId":"47846","inReplyTo":"20180214180322.GA9190@sigill.intra.peff.net","subject":"[PATCH 6/6] test-hashmap: use \"unsigned int\" for hash storage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:08:57Z","receivedAt":"2018-02-14T18:09:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The hashmap API always use an unsigned value for storing\nand comparing hashes. Whereas this test code uses \"int\".\nThis works out in practice since one can typically\nround-trip between \"int\" and \"unsigned int\". But since this\nis essentially reference code for the hashmap API, we should\nmodel using the correct types.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/helper/test-hashmap.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\nindex 56efff36e8..9ae9281c07 100644\n--- a/t/helper/test-hashmap.c\n+++ b/t/helper/test-hashmap.c\n@@ -30,7 +30,8 @@ static int test_entry_cmp(const void *cmp_data,\n \t\treturn strcmp(e1->key, key ? key : e2->key);\n }\n \n-static struct test_entry *alloc_test_entry(int hash, char *key, char *value)\n+static struct test_entry *alloc_test_entry(unsigned int hash,\n+\t\t\t\t\t   char *key, char *value)\n {\n \tsize_t klen = strlen(key);\n \tsize_t vlen = strlen(value);\n@@ -156,7 +157,7 @@ int cmd_main(int argc, const char **argv)\n \t/* process commands from stdin */\n \twhile (strbuf_getline(&line, stdin) != EOF) {\n \t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n-\t\tint hash = 0;\n+\t\tunsigned int hash = 0;\n \t\tstruct test_entry *entry;\n \n \t\t/* break line into command and up to two parameters */\n-- \n2.16.1.464.gc4bae515b7\n"},{"id":"339312","messageId":"CAGZ79kZ5a1-znO_efwVbrRpXwboZHeiC+4PqB0SDR2Xwv5P+8g@mail.gmail.com","threadId":"47846","inReplyTo":"20180214180322.GA9190@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] minor test-hashmap fixes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-02-14T18:41:35Z","receivedAt":"2018-02-14T18:41:42Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Feb 14, 2018 at 10:03 AM, Jeff King <peff@peff.net> wrote:\n> This series started with me fixing the sizeof() mismatch discussed in\n>\n>   https://public-inbox.org/git/20180214164628.GA907@sigill.intra.peff.net/\n>\n> but I found a number of minor irritations. Most of them are cosmetic in\n> practice, but I think it's important for test-helper code like this to\n> model best practices, since people are likely to use it as a reference.\n>\n>   [1/6]: test-hashmap: use ALLOC_ARRAY rather than bare malloc\n>   [2/6]: test-hashmap: check allocation computation for overflow\n>   [3/6]: test-hashmap: use xsnprintf rather than snprintf\n>   [4/6]: test-hashmap: use strbuf_getline rather than fgets\n>   [5/6]: test-hashmap: simplify alloc_test_entry\n>   [6/6]: test-hashmap: use \"unsigned int\" for hash storage\n>\n\nThe whole series is\nReviewed-by: Stefan Beller <sbeller@google.com>\n\nThanks for improving the example code.\nI have lost track of the hashmap improvements lately, but with\nsuch a good test helper, we could reduce the amount of\ncommented code in hashmap.h and just link to the test helper?\n(as an extra step after this series, I thought we already had that)\n\nThanks,\nStefan\n"},{"id":"339313","messageId":"CAGm8dMCdL_KWQSeTdC1B+-XU1E-ix=t0C6tDTdrrdbLEfUWCuw@mail.gmail.com","threadId":"47846","inReplyTo":"20180214180545.GA9919@sigill.intra.peff.net","subject":"Re: [PATCH 1/6] test-hashmap: use ALLOC_ARRAY rather than bare malloc","fromName":"Code AI","fromEmail":"techsupport@mycode.ai","sentAt":"2018-02-14T18:47:48Z","receivedAt":"2018-02-14T18:47:54Z","isPatch":true,"sender":{"key":"techsupport@mycode.ai","avatar":null},"body":"Jeff,\n\nThanks for the feedback!  Most of our users find CodeAI's fixes\nuseful, even if they are not exactly correct.  However, we are\nconstantly working on improving the quality of CodeAI's fixes.  The\nmore people who use it, the more we can improve it.  We would love for\nyou to try the system directly by visiting mycode.ai.  Its free for\nopen source users, and you can easily login using your GitHub account.\nAlso, you may attribute the allocator sizeof() issue to C0deAi.\nThat's the Github account used by the tool to submit pull requests.\n\n-Ben\n\nOn Wed, Feb 14, 2018 at 1:05 PM, Jeff King <peff@peff.net> wrote:\n> These two array allocations have several minor flaws:\n>\n>   - they use bare malloc, rather than our error-checking\n>     xmalloc\n>\n>   - they do a bare multiplication to determine the total\n>     size (which in theory can overflow, though in this case\n>     the sizes are all constants)\n>\n>   - they use sizeof(type), but the type in the second one\n>     doesn't match the actual array (though it's \"int\" versus\n>     \"unsigned int\", which are guaranteed by C99 to have the\n>     same size)\n>\n> None of these are likely to be problems in practice, and\n> this is just a test helper. But since people often look at\n> test helpers as reference code, we should do our best to\n> model the recommended techniques.\n>\n> Switching to ALLOC_ARRAY fixes all three.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The sizeof() thing came from Code AI's original email. I'm happy to\n> include a Reported-by there, but I wasn't sure of the correct entity to\n> credit. :)\n>\n>  t/helper/test-hashmap.c | 4 ++--\n>  1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/helper/test-hashmap.c b/t/helper/test-hashmap.c\n> index 1145d51671..b36886bf35 100644\n> --- a/t/helper/test-hashmap.c\n> +++ b/t/helper/test-hashmap.c\n> @@ -85,8 +85,8 @@ static void perf_hashmap(unsigned int method, unsigned int rounds)\n>         unsigned int *hashes;\n>         unsigned int i, j;\n>\n> -       entries = malloc(TEST_SIZE * sizeof(struct test_entry *));\n> -       hashes = malloc(TEST_SIZE * sizeof(int));\n> +       ALLOC_ARRAY(entries, TEST_SIZE);\n> +       ALLOC_ARRAY(hashes, TEST_SIZE);\n>         for (i = 0; i < TEST_SIZE; i++) {\n>                 snprintf(buf, sizeof(buf), \"%i\", i);\n>                 entries[i] = alloc_test_entry(0, buf, strlen(buf), \"\", 0);\n> --\n> 2.16.1.464.gc4bae515b7\n>\n\n\n\n-- \nSincerely,\n\nCodeAI Tech Support Team\n"},{"id":"339314","messageId":"20180214184846.GA13369@sigill.intra.peff.net","threadId":"47846","inReplyTo":"CAGZ79kZ5a1-znO_efwVbrRpXwboZHeiC+4PqB0SDR2Xwv5P+8g@mail.gmail.com","subject":"Re: [PATCH 0/6] minor test-hashmap fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-14T18:48:46Z","receivedAt":"2018-02-14T18:48:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Feb 14, 2018 at 10:41:35AM -0800, Stefan Beller wrote:\n\n> I have lost track of the hashmap improvements lately, but with\n> such a good test helper, we could reduce the amount of\n> commented code in hashmap.h and just link to the test helper?\n> (as an extra step after this series, I thought we already had that)\n\nYeah, the example code in hashmap.h looks to be more or less the same\nthing as what's here (it maps a long to a value, which simplifies the\nFLEX_ARRAY bits, but I think it serves equally well as a reference).\n\nI'd also be fine with drastically simplifying the example in hashmap.h,\nand letting test-hashmap serve as a more fleshed-out example (we don't\nneed to see the main and scanf bits there).\n\n-Peff\n"},{"id":"339352","messageId":"xmqq3723o0pf.fsf@gitster-ct.c.googlers.com","threadId":"47846","inReplyTo":"20180214180819.GE9919@sigill.intra.peff.net","subject":"Re: [PATCH 5/6] test-hashmap: simplify alloc_test_entry","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-14T19:01:00Z","receivedAt":"2018-02-14T19:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> This function takes two ptr/len pairs, which implies that\n> they can be arbitrary buffers. But internally, it assumes\n> that each \"ptr\" is NUL-terminated at \"len\" (because we\n> memcpy an extra byte to pick up the NUL terminator).\n\nMakes quite a lot of sense.  In addition, get_value() assumes that\nthe key cannot have NUL in it, so the use of counted string in\nalloc_test_entry() serves no purpose other than to confuse the\nreaders ;-)\n\n> Note that we can get rid of the \"l1\" and \"l2\" variables from\n> cmd_main() as a further cleanup, since they are now mostly\n\nMade me wonder if this \"we can\" is \"we could if we wanted to in the\nfuture although we do not go that far in this patch\", but it turns\nout that this is also done, and I think it is a good clean-up.\n"}]}