{"thread":{"id":"51912","subject":"[PATCH 0/3] scan-build fixes","startedAt":"2019-09-25T02:04:47Z","lastAt":"2019-09-29T00:52:01Z","messageCount":13,"participants":["Alex Henrie","Derrick Stolee","Johannes Schindelin","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"382891","messageId":"20190925020158.751492-1-alexhenrie24@gmail.com","threadId":"51912","inReplyTo":null,"subject":"[PATCH 0/3] scan-build fixes","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-25T02:01:55Z","receivedAt":"2019-09-25T02:04:47Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"These patches fix a few problems identified by scan-build [1]. None of\nthem affect behavior, but they do make the code easier to follow and\nensure that the compiler is able to optimize it to the fullest.\n\n-Alex\n\n[1] https://clang-analyzer.llvm.org/scan-build.html\n\nAlex Henrie (3):\n  commit-graph: remove a duplicate assignment\n  diffcore-break: use a goto instead of a redundant if statement\n  wrapper: use a loop instead of repetitive statements\n\n commit-graph.c   |  1 -\n diffcore-break.c | 15 +++++++--------\n wrapper.c        | 11 +++++------\n 3 files changed, 12 insertions(+), 15 deletions(-)\n\n-- \n2.23.0\n\n"},{"id":"382892","messageId":"20190925020158.751492-2-alexhenrie24@gmail.com","threadId":"51912","inReplyTo":"20190925020158.751492-1-alexhenrie24@gmail.com","subject":"[PATCH 1/3] commit-graph: remove a duplicate assignment","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-25T02:01:56Z","receivedAt":"2019-09-25T02:04:49Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n commit-graph.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 9b02d2c426..659f4bb4f4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1534,7 +1534,6 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n \t\tsize_mult = ctx->split_opts->size_multiple;\n \t}\n \n-\tg = ctx->r->objects->commit_graph;\n \tctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;\n \n \twhile (g && (g->num_commits <= size_mult * num_commits ||\n-- \n2.23.0\n\n"},{"id":"382893","messageId":"20190925020158.751492-3-alexhenrie24@gmail.com","threadId":"51912","inReplyTo":"20190925020158.751492-1-alexhenrie24@gmail.com","subject":"[PATCH 2/3] diffcore-break: use a goto instead of a redundant if statement","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-25T02:01:57Z","receivedAt":"2019-09-25T02:04:50Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n diffcore-break.c | 15 +++++++--------\n 1 file changed, 7 insertions(+), 8 deletions(-)\n\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 875aefd3fe..f6ab74141b 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -286,18 +286,17 @@ void diffcore_merge_broken(void)\n \t\t\t\t\t/* Peer survived.  Merge them */\n \t\t\t\t\tmerge_broken(p, pp, &outq);\n \t\t\t\t\tq->queue[j] = NULL;\n-\t\t\t\t\tbreak;\n+\t\t\t\t\tgoto done;\n \t\t\t\t}\n \t\t\t}\n-\t\t\tif (q->nr <= j)\n-\t\t\t\t/* The peer did not survive, so we keep\n-\t\t\t\t * it in the output.\n-\t\t\t\t */\n-\t\t\t\tdiff_q(&outq, p);\n+\t\t\t/* The peer did not survive, so we keep\n+\t\t\t * it in the output.\n+\t\t\t */\n \t\t}\n-\t\telse\n-\t\t\tdiff_q(&outq, p);\n+\t\tdiff_q(&outq, p);\n \t}\n+\n+done:\n \tfree(q->queue);\n \t*q = outq;\n \n-- \n2.23.0\n\n"},{"id":"382894","messageId":"20190925020158.751492-4-alexhenrie24@gmail.com","threadId":"51912","inReplyTo":"20190925020158.751492-1-alexhenrie24@gmail.com","subject":"[PATCH 3/3] wrapper: use a loop instead of repetitive statements","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-25T02:01:58Z","receivedAt":"2019-09-25T02:04:52Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n---\n wrapper.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex c55d7722d7..c23ac6adcd 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \tfilename_template = &pattern[len - 6 - suffix_len];\n \tfor (count = 0; count < TMP_MAX; ++count) {\n \t\tuint64_t v = value;\n+\t\tint i;\n \t\t/* Fill in the random bits. */\n-\t\tfilename_template[0] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[1] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[2] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[3] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[4] = letters[v % num_letters]; v /= num_letters;\n-\t\tfilename_template[5] = letters[v % num_letters]; v /= num_letters;\n+\t\tfor (i = 0; i < 6; i++) {\n+\t\t\tfilename_template[i] = letters[v % num_letters];\n+\t\t\tv /= num_letters;\n+\t\t}\n \n \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n \t\tif (fd >= 0)\n-- \n2.23.0\n\n"},{"id":"382981","messageId":"8307cccf-e115-c720-6670-2ab537066a01@gmail.com","threadId":"51912","inReplyTo":"20190925020158.751492-2-alexhenrie24@gmail.com","subject":"Re: [PATCH 1/3] commit-graph: remove a duplicate assignment","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-26T13:02:01Z","receivedAt":"2019-09-26T13:02:06Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/24/2019 10:01 PM, Alex Henrie wrote:\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>  commit-graph.c | 1 -\n>  1 file changed, 1 deletion(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 9b02d2c426..659f4bb4f4 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -1534,7 +1534,6 @@ static void split_graph_merge_strategy(struct write_commit_graph_context *ctx)\n>  \t\tsize_mult = ctx->split_opts->size_multiple;\n>  \t}\n>  \n> -\tg = ctx->r->objects->commit_graph;\n>  \tctx->num_commit_graphs_after = ctx->num_commit_graphs_before + 1;\n>  \n>  \twhile (g && (g->num_commits <= size_mult * num_commits ||\n\nCould we instead remove the assignment during the declaration? It is\neasier to know the while loop will work if the assignment is closer\nto the loop.\n\nThanks,\n-Stolee\n \n\n"},{"id":"382982","messageId":"5382f6cc-d699-0c99-6d61-60cae45f27c2@gmail.com","threadId":"51912","inReplyTo":"20190925020158.751492-3-alexhenrie24@gmail.com","subject":"Re: [PATCH 2/3] diffcore-break: use a goto instead of a redundant if statement","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-26T13:12:17Z","receivedAt":"2019-09-26T13:12:22Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/24/2019 10:01 PM, Alex Henrie wrote:\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>  diffcore-break.c | 15 +++++++--------\n>  1 file changed, 7 insertions(+), 8 deletions(-)\n> \n> diff --git a/diffcore-break.c b/diffcore-break.c\n> index 875aefd3fe..f6ab74141b 100644\n> --- a/diffcore-break.c\n> +++ b/diffcore-break.c\n> @@ -286,18 +286,17 @@ void diffcore_merge_broken(void)\n>  \t\t\t\t\t/* Peer survived.  Merge them */\n>  \t\t\t\t\tmerge_broken(p, pp, &outq);\n>  \t\t\t\t\tq->queue[j] = NULL;\n> -\t\t\t\t\tbreak;\n> +\t\t\t\t\tgoto done;\n>  \t\t\t\t}\n>  \t\t\t}\n> -\t\t\tif (q->nr <= j)\n> -\t\t\t\t/* The peer did not survive, so we keep\n> -\t\t\t\t * it in the output.\n> -\t\t\t\t */\n> -\t\t\t\tdiff_q(&outq, p);\n> +\t\t\t/* The peer did not survive, so we keep\n> +\t\t\t * it in the output.\n> +\t\t\t */\n>  \t\t}\n> -\t\telse\n> -\t\t\tdiff_q(&outq, p);\n> +\t\tdiff_q(&outq, p);\n>  \t}\n> +\n> +done:\n>  \tfree(q->queue);\n>  \t*q = outq;\n\nThis took a little more context to check.\n\nThe \"if (q->nr <= j)\" condition essentially means \"did the\nfor loop above end via the sentinel, not a break?\" This\nmeans that the diff_q() call is run if the for loop finishes\nnormally OR if we hit the visible \"else\" call.\n\nThere is some subtlety with the if / else if / else conditions.\nThe 'if' condition has a 'continue' which avoids the diff_q in\nits new position.\n\nSomething like the two paragraphs above would be good to add\nto the commit message so we can more easily read this patch\nin the future.\n\nHere is the full method context (in master) to help others:\n\nvoid diffcore_merge_broken(void)\n{\n        struct diff_queue_struct *q = &diff_queued_diff;\n        struct diff_queue_struct outq;\n        int i, j;\n\n        DIFF_QUEUE_CLEAR(&outq);\n\n        for (i = 0; i < q->nr; i++) {\n                struct diff_filepair *p = q->queue[i];\n                if (!p)\n                        /* we already merged this with its peer */\n                        continue;\n                else if (p->broken_pair &&\n                         !strcmp(p->one->path, p->two->path)) {\n                        /* If the peer also survived rename/copy, then\n                         * we merge them back together.\n                         */\n                        for (j = i + 1; j < q->nr; j++) {\n                                struct diff_filepair *pp = q->queue[j];\n                                if (pp->broken_pair &&\n                                    !strcmp(pp->one->path, pp->two->path) &&\n                                    !strcmp(p->one->path, pp->two->path)) {\n                                        /* Peer survived.  Merge them */\n                                        merge_broken(p, pp, &outq);\n                                        q->queue[j] = NULL;\n                                        break;\n                                }\n                        }\n                        if (q->nr <= j)\n                                /* The peer did not survive, so we keep\n                                 * it in the output.\n                                 */\n                                diff_q(&outq, p);\n                }\n                else\n                        diff_q(&outq, p);\n        }\n        free(q->queue);\n        *q = outq;\n\n        return;\n}\n"},{"id":"382984","messageId":"03e509db-942e-4538-4729-4e345df82a7e@gmail.com","threadId":"51912","inReplyTo":"20190925020158.751492-4-alexhenrie24@gmail.com","subject":"Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-26T13:13:08Z","receivedAt":"2019-09-26T13:13:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/24/2019 10:01 PM, Alex Henrie wrote:\n> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> ---\n>  wrapper.c | 11 +++++------\n>  1 file changed, 5 insertions(+), 6 deletions(-)\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index c55d7722d7..c23ac6adcd 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>  \tfilename_template = &pattern[len - 6 - suffix_len];\n>  \tfor (count = 0; count < TMP_MAX; ++count) {\n>  \t\tuint64_t v = value;\n> +\t\tint i;\n>  \t\t/* Fill in the random bits. */\n> -\t\tfilename_template[0] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[1] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[2] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[3] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[4] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[5] = letters[v % num_letters]; v /= num_letters;\n> +\t\tfor (i = 0; i < 6; i++) {\n> +\t\t\tfilename_template[i] = letters[v % num_letters];\n> +\t\t\tv /= num_letters;\n> +\t\t}\n>  \n>  \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n>  \t\tif (fd >= 0)\n> \n\nThis change is clear.\n\nThanks,\n-Stolee\n"},{"id":"383008","messageId":"CAMMLpeTxGiGrgYcC8jbtV-vAeEKMGZD7YRK397mwwpQdHig4KA@mail.gmail.com","threadId":"51912","inReplyTo":"8307cccf-e115-c720-6670-2ab537066a01@gmail.com","subject":"Re: [PATCH 1/3] commit-graph: remove a duplicate assignment","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-26T18:05:01Z","receivedAt":"2019-09-26T18:05:16Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Thu, Sep 26, 2019 at 7:02 AM Derrick Stolee <stolee@gmail.com> wrote:\n>\n> Could we instead remove the assignment during the declaration? It is\n> easier to know the while loop will work if the assignment is closer\n> to the loop.\n\nSure, that's fine. I'll send a v2 with that change.\n\n-Alex\n"},{"id":"383023","messageId":"nycvar.QRO.7.76.6.1909262155020.15067@tvgsbejvaqbjf.bet","threadId":"51912","inReplyTo":"03e509db-942e-4538-4729-4e345df82a7e@gmail.com","subject":"Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-09-26T20:14:17Z","receivedAt":"2019-09-26T20:14:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 26 Sep 2019, Derrick Stolee wrote:\n\n> On 9/24/2019 10:01 PM, Alex Henrie wrote:\n> > Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>\n> > ---\n> >  wrapper.c | 11 +++++------\n> >  1 file changed, 5 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/wrapper.c b/wrapper.c\n> > index c55d7722d7..c23ac6adcd 100644\n> > --- a/wrapper.c\n> > +++ b/wrapper.c\n> > @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n> >  \tfilename_template = &pattern[len - 6 - suffix_len];\n> >  \tfor (count = 0; count < TMP_MAX; ++count) {\n> >  \t\tuint64_t v = value;\n> > +\t\tint i;\n> >  \t\t/* Fill in the random bits. */\n> > -\t\tfilename_template[0] = letters[v % num_letters]; v /= num_letters;\n> > -\t\tfilename_template[1] = letters[v % num_letters]; v /= num_letters;\n> > -\t\tfilename_template[2] = letters[v % num_letters]; v /= num_letters;\n> > -\t\tfilename_template[3] = letters[v % num_letters]; v /= num_letters;\n> > -\t\tfilename_template[4] = letters[v % num_letters]; v /= num_letters;\n> > -\t\tfilename_template[5] = letters[v % num_letters]; v /= num_letters;\n> > +\t\tfor (i = 0; i < 6; i++) {\n> > +\t\t\tfilename_template[i] = letters[v % num_letters];\n> > +\t\t\tv /= num_letters;\n> > +\t\t}\n> >\n> >  \t\tfd = open(pattern, O_CREAT | O_EXCL | O_RDWR, mode);\n> >  \t\tif (fd >= 0)\n> >\n>\n> This change is clear.\n\nNot so fast.\n\nThis looks like it was intended to help compilers that cannot unroll\nloops all that easily, and just because current clang can does not mean\nthat we should put people at a deliberate disadvantage when they are\nstuck with a C compiler that cannot optimize the post-image of this\ndiff.\n\nLet's first see whether my gut feeling has any merit.\n\nThis part of the code entered Git's tree in 0620b39b3b7 (compat: add a\nmkstemps() compatibility function, 2009-05-31), and it was clearly\ncopy-edited from libiberty.\n\nIt entered libiberty in\nhttps://github.com/gcc-mirror/gcc/commit/119735e34916. That commit\nclaims that it was copy-edited from glibc, and edited it was, as it\ninlined the `__gen_tempname()` function.\n\nSadly, the commit message of the patch that introduced this pattern into\nglibc is pretty much not helpful at all here:\nhttps://sourceware.org/git/?p=glibc.git;a=commitdiff;h=a7ab2023fcdd5c90c9f664cbaed8ef90dd38e818\n(it only talks about using a random-name generator that is already used\nelsewhere.)\n\nIn short: sadly, this history excursion did not reveal anything to back\nup my intuition that your change would revert a change that might be\ncrucial on older platforms.\n\nHowever, I think that this patch should at least be accompanied by a\ncommit message that suggests that some thought was put into it, and that\nconcerns like mine were considered carefully.\n\nI mean, if there is _any_ performance-critical code path hitting this\nunrolled loop, we may want to keep it unrolled.\n\nCiao,\nJohannes\n"},{"id":"383050","messageId":"20190927024502.GC23736@sigill.intra.peff.net","threadId":"51912","inReplyTo":"20190925020158.751492-4-alexhenrie24@gmail.com","subject":"Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-27T02:45:03Z","receivedAt":"2019-09-27T02:45:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 24, 2019 at 08:01:58PM -0600, Alex Henrie wrote:\n\n> diff --git a/wrapper.c b/wrapper.c\n> index c55d7722d7..c23ac6adcd 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>  \tfilename_template = &pattern[len - 6 - suffix_len];\n>  \tfor (count = 0; count < TMP_MAX; ++count) {\n>  \t\tuint64_t v = value;\n> +\t\tint i;\n>  \t\t/* Fill in the random bits. */\n> -\t\tfilename_template[0] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[1] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[2] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[3] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[4] = letters[v % num_letters]; v /= num_letters;\n> -\t\tfilename_template[5] = letters[v % num_letters]; v /= num_letters;\n> +\t\tfor (i = 0; i < 6; i++) {\n> +\t\t\tfilename_template[i] = letters[v % num_letters];\n> +\t\t\tv /= num_letters;\n> +\t\t}\n\nI'm not sure the readability is changed much either way. But it does\nenable this additional cleanup on top:\n\n-- >8 --\nSubject: git_mkstemps_mode(): replace magic numbers with computed value\n\nThe magic number \"6\" appears several times in the function, and is\nrelated to the size of the \"XXXXXX\" string we expect to find in the\ntemplate. Let's pull that \"XXXXXX\" into a constant array, whose size we\ncan get at compile time with ARRAY_SIZE().\n\nNote that we probably can't just change this value, since callers will\nbe feeding us a certain number of X's, but it hopefully makes the\nfunction itself easier to follow.\n\nWhile we're here, let's do the same with the \"letters\" array (which we\n_could_ modify if we wanted to include more characters).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wrapper.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex c23ac6adcd..e1eaef2e16 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -441,7 +441,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t\t\"abcdefghijklmnopqrstuvwxyz\"\n \t\t\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n \t\t\"0123456789\";\n-\tstatic const int num_letters = 62;\n+\tstatic const int num_letters = ARRAY_SIZE(letters) - 1;\n+\tstatic const char x_pattern[] = \"XXXXXX\";\n+\tstatic const int num_x = ARRAY_SIZE(x_pattern) - 1;\n \tuint64_t value;\n \tstruct timeval tv;\n \tchar *filename_template;\n@@ -450,12 +452,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \n \tlen = strlen(pattern);\n \n-\tif (len < 6 + suffix_len) {\n+\tif (len < num_x + suffix_len) {\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n \n-\tif (strncmp(&pattern[len - 6 - suffix_len], \"XXXXXX\", 6)) {\n+\tif (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) {\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n@@ -466,12 +468,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n \t */\n \tgettimeofday(&tv, NULL);\n \tvalue = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();\n-\tfilename_template = &pattern[len - 6 - suffix_len];\n+\tfilename_template = &pattern[len - num_x - suffix_len];\n \tfor (count = 0; count < TMP_MAX; ++count) {\n \t\tuint64_t v = value;\n \t\tint i;\n \t\t/* Fill in the random bits. */\n-\t\tfor (i = 0; i < 6; i++) {\n+\t\tfor (i = 0; i < num_x; i++) {\n \t\t\tfilename_template[i] = letters[v % num_letters];\n \t\t\tv /= num_letters;\n \t\t}\n-- \n2.23.0.765.g1fc3e247e7\n\n"},{"id":"383051","messageId":"20190927025000.GD23736@sigill.intra.peff.net","threadId":"51912","inReplyTo":"nycvar.QRO.7.76.6.1909262155020.15067@tvgsbejvaqbjf.bet","subject":"Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-27T02:50:01Z","receivedAt":"2019-09-27T02:50:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 26, 2019 at 10:14:17PM +0200, Johannes Schindelin wrote:\n\n> I mean, if there is _any_ performance-critical code path hitting this\n> unrolled loop, we may want to keep it unrolled.\n\nThe loop in question is maybe a few dozen instructions, and then it\nimmediately makes an open() syscall, which is probably multiple orders\nof magnitude more expensive. So I'd be very surprised if it was a\nproblem no matter what the generated code looks like.\n\nBut...\n\n> However, I think that this patch should at least be accompanied by a\n> commit message that suggests that some thought was put into it, and that\n> concerns like mine were considered carefully.\n\n...this I would definitely agree with.\n\n-Peff\n"},{"id":"383059","messageId":"d26869f8-9373-51a1-a146-7367aeeb2e85@gmail.com","threadId":"51912","inReplyTo":"20190927024502.GC23736@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-09-27T11:48:46Z","receivedAt":"2019-09-27T11:48:52Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/26/2019 10:45 PM, Jeff King wrote:\n> On Tue, Sep 24, 2019 at 08:01:58PM -0600, Alex Henrie wrote:\n> \n>> diff --git a/wrapper.c b/wrapper.c\n>> index c55d7722d7..c23ac6adcd 100644\n>> --- a/wrapper.c\n>> +++ b/wrapper.c\n>> @@ -469,13 +469,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>>  \tfilename_template = &pattern[len - 6 - suffix_len];\n>>  \tfor (count = 0; count < TMP_MAX; ++count) {\n>>  \t\tuint64_t v = value;\n>> +\t\tint i;\n>>  \t\t/* Fill in the random bits. */\n>> -\t\tfilename_template[0] = letters[v % num_letters]; v /= num_letters;\n>> -\t\tfilename_template[1] = letters[v % num_letters]; v /= num_letters;\n>> -\t\tfilename_template[2] = letters[v % num_letters]; v /= num_letters;\n>> -\t\tfilename_template[3] = letters[v % num_letters]; v /= num_letters;\n>> -\t\tfilename_template[4] = letters[v % num_letters]; v /= num_letters;\n>> -\t\tfilename_template[5] = letters[v % num_letters]; v /= num_letters;\n>> +\t\tfor (i = 0; i < 6; i++) {\n>> +\t\t\tfilename_template[i] = letters[v % num_letters];\n>> +\t\t\tv /= num_letters;\n>> +\t\t}\n> \n> I'm not sure the readability is changed much either way. But it does\n> enable this additional cleanup on top:\n> \n> -- >8 --\n> Subject: git_mkstemps_mode(): replace magic numbers with computed value\n> \n> The magic number \"6\" appears several times in the function, and is\n> related to the size of the \"XXXXXX\" string we expect to find in the\n> template. Let's pull that \"XXXXXX\" into a constant array, whose size we\n> can get at compile time with ARRAY_SIZE().\n\nRemoving magic numbers is always a good change. Thanks!\n\n> Note that we probably can't just change this value, since callers will\n> be feeding us a certain number of X's, but it hopefully makes the\n> function itself easier to follow.\n> \n> While we're here, let's do the same with the \"letters\" array (which we\n> _could_ modify if we wanted to include more characters).\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  wrapper.c | 12 +++++++-----\n>  1 file changed, 7 insertions(+), 5 deletions(-)\n> \n> diff --git a/wrapper.c b/wrapper.c\n> index c23ac6adcd..e1eaef2e16 100644\n> --- a/wrapper.c\n> +++ b/wrapper.c\n> @@ -441,7 +441,9 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>  \t\t\"abcdefghijklmnopqrstuvwxyz\"\n>  \t\t\"ABCDEFGHIJKLMNOPQRSTUVWXYZ\"\n>  \t\t\"0123456789\";\n> -\tstatic const int num_letters = 62;\n> +\tstatic const int num_letters = ARRAY_SIZE(letters) - 1;\n> +\tstatic const char x_pattern[] = \"XXXXXX\";\n> +\tstatic const int num_x = ARRAY_SIZE(x_pattern) - 1;\n>  \tuint64_t value;\n>  \tstruct timeval tv;\n>  \tchar *filename_template;\n> @@ -450,12 +452,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>  \n>  \tlen = strlen(pattern);\n>  \n> -\tif (len < 6 + suffix_len) {\n> +\tif (len < num_x + suffix_len) {\n>  \t\terrno = EINVAL;\n>  \t\treturn -1;\n>  \t}\n>  \n> -\tif (strncmp(&pattern[len - 6 - suffix_len], \"XXXXXX\", 6)) {\n> +\tif (strncmp(&pattern[len - num_x - suffix_len], x_pattern, num_x)) {\n>  \t\terrno = EINVAL;\n>  \t\treturn -1;\n>  \t}\n> @@ -466,12 +468,12 @@ int git_mkstemps_mode(char *pattern, int suffix_len, int mode)\n>  \t */\n>  \tgettimeofday(&tv, NULL);\n>  \tvalue = ((uint64_t)tv.tv_usec << 16) ^ tv.tv_sec ^ getpid();\n> -\tfilename_template = &pattern[len - 6 - suffix_len];\n> +\tfilename_template = &pattern[len - num_x - suffix_len];\n>  \tfor (count = 0; count < TMP_MAX; ++count) {\n>  \t\tuint64_t v = value;\n>  \t\tint i;\n>  \t\t/* Fill in the random bits. */\n> -\t\tfor (i = 0; i < 6; i++) {\n> +\t\tfor (i = 0; i < num_x; i++) {\n>  \t\t\tfilename_template[i] = letters[v % num_letters];\n>  \t\t\tv /= num_letters;\n>  \t\t}\n> \n\n"},{"id":"383091","messageId":"CAMMLpeRva+sVxhTE14prMFvZnJhouQO+vDYeze9ZRao-dAfsFA@mail.gmail.com","threadId":"51912","inReplyTo":"20190927025000.GD23736@sigill.intra.peff.net","subject":"Re: [PATCH 3/3] wrapper: use a loop instead of repetitive statements","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2019-09-29T00:51:46Z","receivedAt":"2019-09-29T00:52:01Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Thu, Sep 26, 2019 at 8:50 PM Jeff King <peff@peff.net> wrote:\n>\n> On Thu, Sep 26, 2019 at 10:14:17PM +0200, Johannes Schindelin wrote:\n>\n> > However, I think that this patch should at least be accompanied by a\n> > commit message that suggests that some thought was put into it, and that\n> > concerns like mine were considered carefully.\n>\n> ...this I would definitely agree with.\n\nThanks for the feedback everyone. I am not used to projects requiring\ndetailed commit messages for small changes. I will send a v3 of the\nthree patches with the additional explanations you requested.\n\n-Alex\n"}]}