{"thread":{"id":"63188","subject":"[PATCH 0/2] Avoid the comma operator","startedAt":"2025-03-25T08:01:55Z","lastAt":"2025-03-29T00:39:25Z","messageCount":64,"participants":["Johannes Schindelin via GitGitGadget","Philip Oakley","Patrick Steinhardt","Johannes Schindelin","Phillip Wood","Karthik Nayak","Jeff King","Junio C Hamano","Taylor Blau","Eric Sunshine","Chris Torek"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"514982","messageId":"pull.1889.git.1742889711.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":null,"subject":"[PATCH 0/2] Avoid the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T08:01:48Z","receivedAt":"2025-03-25T08:01:55Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The comma operator\n[https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\nrarely used in C anymore, and typically indicates a typo. Just like in these\ninstances, where a semicolon was meant to be used, as there is no need to\ndiscard the first statement's result here.\n\nJohannes Schindelin (2):\n remote-curl: avoid using the comma operator unnecessarily\n rebase: avoid using the comma operator unnecessarily\n\n builtin/rebase.c | 2 +-\n remote-curl.c | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1889\n-- \ngitgitgadget\n"},{"id":"514983","messageId":"e3069fd4564bac68bdaf2079151b9b921867e277.1742889711.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.git.1742889711.gitgitgadget@gmail.com","subject":"[PATCH 1/2] remote-curl: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T08:01:49Z","receivedAt":"2025-03-25T08:01:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n remote-curl.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1273507a96c..57b515b37e7 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-receive-pack\",\n+\trpc.service_name = \"git-receive-pack\";\n \n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n \tif (rpc_result.len)\n-- \ngitgitgadget\n\n"},{"id":"514984","messageId":"7dfbdc48954b55a435c8cb429b648d77a1a9d044.1742889711.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.git.1742889711.gitgitgadget@gmail.com","subject":"[PATCH 2/2] rebase: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T08:01:50Z","receivedAt":"2025-03-25T08:01:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d4715ed35d7..62bdf7276f7 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,\n \tstrbuf_addf(&msg, \"%s (start): checkout %s\",\n \t\t options.reflog_action, options.onto_name);\n \tropts.oid = &options.onto->object.oid;\n-\tropts.orig_head = &options.orig_head->object.oid,\n+\tropts.orig_head = &options.orig_head->object.oid;\n \tropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\n-- \ngitgitgadget\n"},{"id":"514992","messageId":"f30d8ac4-4748-4b73-b86d-6f85c9bac622@iee.email","threadId":"63188","inReplyTo":"pull.1889.git.1742889711.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2025-03-25T11:41:40Z","receivedAt":"2025-03-25T12:12:28Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:\n> The comma operator\n> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> rarely used in C anymore, and typically indicates a typo. Just like in these\n> instances, where a semicolon was meant to be used, as there is no need to\n> discard the first statement's result here.\n\nMinor aside: How were these 'discovered'?\n\n> \n> Johannes Schindelin (2):\n> remote-curl: avoid using the comma operator unnecessarily\n> rebase: avoid using the comma operator unnecessarily\n> \n> builtin/rebase.c | 2 +-\n> remote-curl.c | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n> \n> \n> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1889\n\nPhilip\n"},{"id":"514993","messageId":"Z-Kf9ovF8zAemgUP@pks.im","threadId":"63188","inReplyTo":"pull.1889.git.1742889711.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-25T12:22:14Z","receivedAt":"2025-03-25T12:22:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 25, 2025 at 08:01:48AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> The comma operator\n> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> rarely used in C anymore, and typically indicates a typo. Just like in these\n> instances, where a semicolon was meant to be used, as there is no need to\n> discard the first statement's result here.\n\nThe changes look obviously good to me, thanks. The reftable library and\nbackend also had several instances where the operator was used by\naccident, and I've gotten rid of those over time. They typically don't\ndo any harm as the result is essentially the same, but sometimes they\nmay cause issues. And at the very least they cause confusion.\n\nIt would be great if there was a compiler warning we could enable for\ncases where the operator likely isn't intentional. But I couldn't find\nany, unfortunately.\n\nThanks!\n\nPatrick\n"},{"id":"515020","messageId":"ea6bfd92-3d09-c611-abdc-dc7bc987446f@gmx.de","threadId":"63188","inReplyTo":"f30d8ac4-4748-4b73-b86d-6f85c9bac622@iee.email","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-25T14:12:21Z","receivedAt":"2025-03-25T14:12:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Philip,\n\nOn Tue, 25 Mar 2025, Philip Oakley wrote:\n\n> On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:\n> > The comma operator\n> > [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> > rarely used in C anymore, and typically indicates a typo. Just like in these\n> > instances, where a semicolon was meant to be used, as there is no need to\n> > discard the first statement's result here.\n>\n> Minor aside: How were these 'discovered'?\n\nI am working on a GitHub workflow that uses CodeQL to find such issues,\nthat's how I found them. (I also worked with the CodeQL team to get this\nquery added, way back when I was still working at GitHub.)\n\nCiao,\nJohannes\n"},{"id":"515021","messageId":"22fe76ff-7940-86bd-af50-c161e3d04864@gmx.de","threadId":"63188","inReplyTo":"Z-Kf9ovF8zAemgUP@pks.im","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-25T14:13:47Z","receivedAt":"2025-03-25T14:13:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Tue, 25 Mar 2025, Patrick Steinhardt wrote:\n\n> On Tue, Mar 25, 2025 at 08:01:48AM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > The comma operator\n> > [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> > rarely used in C anymore, and typically indicates a typo. Just like in these\n> > instances, where a semicolon was meant to be used, as there is no need to\n> > discard the first statement's result here.\n>\n> The changes look obviously good to me, thanks. The reftable library and\n> backend also had several instances where the operator was used by\n> accident, and I've gotten rid of those over time. They typically don't\n> do any harm as the result is essentially the same, but sometimes they\n> may cause issues. And at the very least they cause confusion.\n\nThank you for addressing these in the reftable library!\n\n> It would be great if there was a compiler warning we could enable for\n> cases where the operator likely isn't intentional. But I couldn't find\n> any, unfortunately.\n\nI was not actually planning on adding the CodeQL workflow to git/git,\nseeing as its CI is already taking way too much CPU time for my liking.\nBut in `microsoft/git`, I am kind of required to, so we'll catch those\nissues there.\n\nCiao,\nJohannes\n"},{"id":"515024","messageId":"b7145e94-f5c5-496e-9d36-0022841ceae6@gmail.com","threadId":"63188","inReplyTo":"7dfbdc48954b55a435c8cb429b648d77a1a9d044.1742889711.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] rebase: avoid using the comma operator unnecessarily","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-25T14:35:44Z","receivedAt":"2025-03-25T14:35:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n > > diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index d4715ed35d7..62bdf7276f7 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,\n> \tstrbuf_addf(&msg, \"%s (start): checkout %s\",\n> \t\t options.reflog_action, options.onto_name);\n> \tropts.oid = &options.onto->object.oid;\n> -\tropts.orig_head = &options.orig_head->object.oid,\n> +\tropts.orig_head = &options.orig_head->object.oid;\n\nThis is obviously a typo - thanks for fixing it\n\nBest Wishes\n\nPhillip\n\n> \tropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n> \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n> \tropts.head_msg = msg.buf;\n\n"},{"id":"515026","messageId":"CAOLa=ZQqknrcrkc=kGsybB5TJPgxH5HKAtL3HMu1uKYkX0NicQ@mail.gmail.com","threadId":"63188","inReplyTo":"Z-Kf9ovF8zAemgUP@pks.im","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-25T14:37:00Z","receivedAt":"2025-03-25T14:37:02Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Mar 25, 2025 at 08:01:48AM +0000, Johannes Schindelin via GitGitGadget wrote:\n>> The comma operator\n>> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n>> rarely used in C anymore, and typically indicates a typo. Just like in these\n>> instances, where a semicolon was meant to be used, as there is no need to\n>> discard the first statement's result here.\n>\n> The changes look obviously good to me, thanks. The reftable library and\n> backend also had several instances where the operator was used by\n> accident, and I've gotten rid of those over time. They typically don't\n> do any harm as the result is essentially the same, but sometimes they\n> may cause issues. And at the very least they cause confusion.\n>\n> It would be great if there was a compiler warning we could enable for\n> cases where the operator likely isn't intentional. But I couldn't find\n> any, unfortunately.\n>\n\nClang does have '-Wcomma' [1], which seems to be exactly what we want.\n\n[1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wcomma\n\n> Thanks!\n>\n> Patrick\n"},{"id":"515030","messageId":"930b4c9a-826f-4124-a70b-e0400a3fea5b@gmail.com","threadId":"63188","inReplyTo":"e3069fd4564bac68bdaf2079151b9b921867e277.1742889711.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] remote-curl: avoid using the comma operator unnecessarily","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-25T16:28:32Z","receivedAt":"2025-03-25T16:28:36Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The comma operator is a somewhat obscure C feature that is often used by\n> mistake and can even cause unintentional code flow. Better use a\n> semicolon instead.\n\nclang's -Wcomma finds another instance in this file\n\n@@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads,\n \tpacket_buf_flush(&preamble);\n\n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-upload-pack\",\n+\trpc.service_name = \"git-upload-pack\";\n \trpc.gzip_request = 1;\n\n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n\nIn fact it finds a surprising number in our code base. I was worried \nthere would be a lot of false positives but so far all of the ones I've \nlooked at would be better not using a \",\"\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Johannes Schindelin \n> ---\n> remote-curl.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/remote-curl.c b/remote-curl.c\n> index 1273507a96c..57b515b37e7 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n> \tpacket_buf_flush(&preamble);\n> \n> \tmemset(&rpc, 0, sizeof(rpc));\n> -\trpc.service_name = \"git-receive-pack\",\n> +\trpc.service_name = \"git-receive-pack\";\n> \n> \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n> \tif (rpc_result.len)\n\n"},{"id":"515031","messageId":"20250325165514.GA1902258@coredump.intra.peff.net","threadId":"63188","inReplyTo":"930b4c9a-826f-4124-a70b-e0400a3fea5b@gmail.com","subject":"Re: [PATCH 1/2] remote-curl: avoid using the comma operator unnecessarily","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-25T16:55:14Z","receivedAt":"2025-03-25T16:55:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 25, 2025 at 04:28:32PM +0000, Phillip Wood wrote:\n\n> On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> > \n> > The comma operator is a somewhat obscure C feature that is often used by\n> > mistake and can even cause unintentional code flow. Better use a\n> > semicolon instead.\n> \n> clang's -Wcomma finds another instance in this file\n> \n> @@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads,\n> \tpacket_buf_flush(&preamble);\n> \n> \tmemset(&rpc, 0, sizeof(rpc));\n> -\trpc.service_name = \"git-upload-pack\",\n> +\trpc.service_name = \"git-upload-pack\";\n> \trpc.gzip_request = 1;\n> \n> \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n> \n> In fact it finds a surprising number in our code base. I was worried there\n> would be a lot of false positives but so far all of the ones I've looked at\n> would be better not using a \",\"\n\nIt looks like there are a few tricky cases. Inside a loop condition,\na comma can be used to smuggle in an extra line. E.g., in wildmatch:\n\n do {\n ...\n } while (prev_ch = p_ch, (p_ch = *++p) != ']');\n\nor there are a few spots in clar like:\n\n while ((d = (errno = 0, readdir(source_dir))) != NULL) {\n ...\n }\n cl_assert_(errno == 0, \"Failed to iterate source dir\");\n\nThese could probably be re-written, but it's not a totally trivial\nchange.\n\nThe rest of them seem pretty straight-forward (though you do have to\nwatch out for conditionals using it to stuff multiple lines into a\nsingle \"statement\"):\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d4715ed35d..62bdf7276f 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,\n \tstrbuf_addf(&msg, \"%s (start): checkout %s\",\n \t\t options.reflog_action, options.onto_name);\n \tropts.oid = &options.onto->object.oid;\n-\tropts.orig_head = &options.orig_head->object.oid,\n+\tropts.orig_head = &options.orig_head->object.oid;\n \tropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\ndiff --git a/diff-delta.c b/diff-delta.c\nindex a4faf73829..71d37368d6 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -438,19 +438,31 @@ create_delta(const struct delta_index *index,\n \t\t\top = out + outpos++;\n \t\t\ti = 0x80;\n \n-\t\t\tif (moff & 0x000000ff)\n-\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n-\t\t\tif (moff & 0x0000ff00)\n-\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n-\t\t\tif (moff & 0x00ff0000)\n-\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n-\t\t\tif (moff & 0xff000000)\n-\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n-\n-\t\t\tif (msize & 0x00ff)\n-\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n-\t\t\tif (msize & 0xff00)\n-\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n+\t\t\tif (moff & 0x000000ff) {\n+\t\t\t\tout[outpos++] = moff >> 0;\n+\t\t\t\ti |= 0x01;\n+\t\t\t}\n+\t\t\tif (moff & 0x0000ff00) {\n+\t\t\t\tout[outpos++] = moff >> 8;\n+\t\t\t\ti |= 0x02;\n+\t\t\t}\n+\t\t\tif (moff & 0x00ff0000) {\n+\t\t\t\tout[outpos++] = moff >> 16;\n+\t\t\t\ti |= 0x04;\n+\t\t\t}\n+\t\t\tif (moff & 0xff000000) {\n+\t\t\t\tout[outpos++] = moff >> 24;\n+\t\t\t\ti |= 0x08;\n+\t\t\t}\n+\n+\t\t\tif (msize & 0x00ff) {\n+\t\t\t\tout[outpos++] = msize >> 0;\n+\t\t\t\ti |= 0x10;\n+\t\t\t}\n+\t\t\tif (msize & 0xff00) {\n+\t\t\t\tout[outpos++] = msize >> 8;\n+\t\t\t\ti |= 0x20;\n+\t\t\t}\n \n \t\t\t*op = i;\n \ndiff --git a/kwset.c b/kwset.c\nindex 1714eada60..23ab015448 100644\n--- a/kwset.c\n+++ b/kwset.c\n@@ -197,10 +197,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n while (link && label != link->label)\n \t{\n \t links[depth] = link;\n-\t if (label < link->label)\n-\t dirs[depth++] = L, link = link->llink;\n-\t else\n-\t dirs[depth++] = R, link = link->rlink;\n+\t if (label < link->label) {\n+\t dirs[depth++] = L;\n+\t link = link->llink;\n+\t }\n+\t else {\n+\t dirs[depth++] = R;\n+\t link = link->rlink;\n+\t }\n \t}\n \n /* The current character doesn't have an outgoing link at\n@@ -257,14 +261,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n \t\t switch (dirs[depth + 1])\n \t\t {\n \t\t case L:\n-\t\t r = links[depth], t = r->llink, rl = t->rlink;\n-\t\t t->rlink = r, r->llink = rl;\n+\t\t r = links[depth]; t = r->llink; rl = t->rlink;\n+\t\t t->rlink = r; r->llink = rl;\n \t\t t->balance = r->balance = 0;\n \t\t break;\n \t\t case R:\n-\t\t r = links[depth], l = r->llink, t = l->rlink;\n-\t\t rl = t->rlink, lr = t->llink;\n-\t\t t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;\n+\t\t r = links[depth]; l = r->llink; t = l->rlink;\n+\t\t rl = t->rlink; lr = t->llink;\n+\t\t t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;\n \t\t l->balance = t->balance != 1 ? 0 : -1;\n \t\t r->balance = t->balance != (char) -1 ? 0 : 1;\n \t\t t->balance = 0;\n@@ -277,14 +281,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n \t\t switch (dirs[depth + 1])\n \t\t {\n \t\t case R:\n-\t\t l = links[depth], t = l->rlink, lr = t->llink;\n-\t\t t->llink = l, l->rlink = lr;\n+\t\t l = links[depth]; t = l->rlink; lr = t->llink;\n+\t\t t->llink = l; l->rlink = lr;\n \t\t t->balance = l->balance = 0;\n \t\t break;\n \t\t case L:\n-\t\t l = links[depth], r = l->rlink, t = r->llink;\n-\t\t lr = t->llink, rl = t->rlink;\n-\t\t t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;\n+\t\t l = links[depth]; r = l->rlink; t = r->llink;\n+\t\t lr = t->llink; rl = t->rlink;\n+\t\t t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;\n \t\t l->balance = t->balance != 1 ? 0 : -1;\n \t\t r->balance = t->balance != (char) -1 ? 0 : 1;\n \t\t t->balance = 0;\n@@ -567,22 +571,22 @@ bmexec (kwset_t kws, char const *text, size_t size)\n {\n \twhile (tp <= ep)\n \t {\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t }\n \tbreak;\n found:\n@@ -649,7 +653,7 @@ cwexec (kwset_t kws, char const *text, size_t len, struct kwsmatch *kwsmatch)\n mch = NULL;\n else\n {\n- mch = text, accept = kwset->trie;\n+ mch = text; accept = kwset->trie;\n goto match;\n }\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1273507a96..590b228f67 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads,\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-upload-pack\",\n+\trpc.service_name = \"git-upload-pack\";\n \trpc.gzip_request = 1;\n \n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n@@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-receive-pack\",\n+\trpc.service_name = \"git-receive-pack\";\n \n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n \tif (rpc_result.len)\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 8889b8b62a..5a96e36dfb 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -211,8 +211,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n \t\t\tfor (d = fmax; d >= fmin; d -= 2) {\n \t\t\t\ti1 = XDL_MIN(kvdf[d], lim1);\n \t\t\t\ti2 = i1 - d;\n-\t\t\t\tif (lim2 < i2)\n-\t\t\t\t\ti1 = lim2 + d, i2 = lim2;\n+\t\t\t\tif (lim2 < i2) {\n+\t\t\t\t\ti1 = lim2 + d;\n+\t\t\t\t\ti2 = lim2;\n+\t\t\t\t}\n \t\t\t\tif (fbest < i1 + i2) {\n \t\t\t\t\tfbest = i1 + i2;\n \t\t\t\t\tfbest1 = i1;\n@@ -223,8 +225,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n \t\t\tfor (d = bmax; d >= bmin; d -= 2) {\n \t\t\t\ti1 = XDL_MAX(off1, kvdb[d]);\n \t\t\t\ti2 = i1 - d;\n-\t\t\t\tif (i2 < off2)\n-\t\t\t\t\ti1 = off2 + d, i2 = off2;\n+\t\t\t\tif (i2 < off2) {\n+\t\t\t\t\ti1 = off2 + d;\n+\t\t\t\t\ti2 = off2;\n+\t\t\t\t}\n \t\t\t\tif (i1 + i2 < bbest) {\n \t\t\t\t\tbbest = i1 + i2;\n \t\t\t\t\tbbest1 = i1;\n\n-Peff\n"},{"id":"515035","messageId":"xmqqh63gnbum.fsf@gitster.g","threadId":"63188","inReplyTo":"pull.1889.git.1742889711.gitgitgadget@gmail.com","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-25T19:34:25Z","receivedAt":"2025-03-25T19:34:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> The comma operator\n> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> rarely used in C anymore, and typically indicates a typo. Just like in these\n> instances, where a semicolon was meant to be used, as there is no need to\n> discard the first statement's result here.\n>\n> Johannes Schindelin (2):\n> remote-curl: avoid using the comma operator unnecessarily\n> rebase: avoid using the comma operator unnecessarily\n\nWell spotted.\n\nThese two looked somehow surprisingly bad.\n\nIf I hadn't known better, I may have spent quite some time wondering\nif these are some ways to hide an unexpected behaviour behind the\ndifferences between a comma and a semicolon for nefarious purposes.\n\nWill queue. Thanks.\n\n>\n> builtin/rebase.c | 2 +-\n> remote-curl.c | 2 +-\n> 2 files changed, 2 insertions(+), 2 deletions(-)\n>\n>\n> base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1889\n"},{"id":"515052","messageId":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.git.1742889711.gitgitgadget@gmail.com","subject":"[PATCH v2 00/10] Avoid the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:04Z","receivedAt":"2025-03-25T23:32:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The comma operator\n[https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\nrarely used in C anymore, and typically indicates a typo. Just like in these\ninstances, where a semicolon was meant to be used, as there is no need to\ndiscard the first statement's result here.\n\nChanges since v1:\n\n * Use -Wcomma when compiling with clang and with DEVELOPER=1.\n * Address the remaining instances pointed out by clang (and by Phillip).\n\nJohannes Schindelin (10):\n remote-curl: avoid using the comma operator unnecessarily\n rebase: avoid using the comma operator unnecessarily\n kwset: avoid using the comma operator unnecessarily\n clar: avoid using the comma operator unnecessarily\n xdiff: avoid using the comma operator unnecessarily\n diff-delta: explicitly mark intentional use of the comma operator\n wildmatch: explicitly mark intentional use of the comma operator\n compat/regex: explicitly mark intentional use of the comma operator\n clang: warn when the comma operator is used\n detect-compiler: detect clang even if it found CUDA\n\n builtin/rebase.c | 2 +-\n compat/regex/regex_internal.c | 7 +++--\n compat/regex/regexec.c | 2 +-\n config.mak.dev | 4 +++\n detect-compiler | 2 +-\n diff-delta.c | 12 ++++----\n kwset.c | 54 +++++++++++++++++++----------------\n remote-curl.c | 4 +--\n t/unit-tests/clar/clar/fs.h | 10 +++++--\n wildmatch.c | 2 +-\n xdiff/xdiffi.c | 12 +++++---\n 11 files changed, 65 insertions(+), 46 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1889\n\nRange-diff vs v1:\n\n 1: e3069fd4564 ! 1: 913c7a0d296 remote-curl: avoid using the comma operator unnecessarily\n @@ Commit message\n Signed-off-by: Johannes Schindelin \n \n ## remote-curl.c ##\n +@@ remote-curl.c: static int fetch_git(struct discovery *heads,\n + \tpacket_buf_flush(&preamble);\n + \n + \tmemset(&rpc, 0, sizeof(rpc));\n +-\trpc.service_name = \"git-upload-pack\",\n ++\trpc.service_name = \"git-upload-pack\";\n + \trpc.gzip_request = 1;\n + \n + \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n @@ remote-curl.c: static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \tpacket_buf_flush(&preamble);\n \n 2: 7dfbdc48954 = 2: 37ff88b8275 rebase: avoid using the comma operator unnecessarily\n -: ----------- > 3: f601f4e74a5 kwset: avoid using the comma operator unnecessarily\n -: ----------- > 4: f60ebe376e1 clar: avoid using the comma operator unnecessarily\n -: ----------- > 5: 7239078413f xdiff: avoid using the comma operator unnecessarily\n -: ----------- > 6: 5e0e8325620 diff-delta: explicitly mark intentional use of the comma operator\n -: ----------- > 7: 9a6de12b807 wildmatch: explicitly mark intentional use of the comma operator\n -: ----------- > 8: dc626f36df3 compat/regex: explicitly mark intentional use of the comma operator\n -: ----------- > 9: 91f86c3aba9 clang: warn when the comma operator is used\n -: ----------- > 10: 2f6f31240fe detect-compiler: detect clang even if it found CUDA\n\n-- \ngitgitgadget\n"},{"id":"515053","messageId":"913c7a0d29699c2bd80fefcdc00879c400f1e7db.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 01/10] remote-curl: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:05Z","receivedAt":"2025-03-25T23:32:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n remote-curl.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1273507a96c..590b228f67f 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads,\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-upload-pack\",\n+\trpc.service_name = \"git-upload-pack\";\n \trpc.gzip_request = 1;\n \n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n@@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-receive-pack\",\n+\trpc.service_name = \"git-receive-pack\";\n \n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n \tif (rpc_result.len)\n-- \ngitgitgadget\n\n"},{"id":"515054","messageId":"37ff88b8275cf4d6b0c715a99f4572e70d6e3729.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 02/10] rebase: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:06Z","receivedAt":"2025-03-25T23:32:19Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d4715ed35d7..62bdf7276f7 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,\n \tstrbuf_addf(&msg, \"%s (start): checkout %s\",\n \t\t options.reflog_action, options.onto_name);\n \tropts.oid = &options.onto->object.oid;\n-\tropts.orig_head = &options.orig_head->object.oid,\n+\tropts.orig_head = &options.orig_head->object.oid;\n \tropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\n-- \ngitgitgadget\n\n"},{"id":"515055","messageId":"f601f4e74a5e8146f2926a725f3d1522b928eb51.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 03/10] kwset: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:07Z","receivedAt":"2025-03-25T23:32:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n kwset.c | 54 +++++++++++++++++++++++++++++-------------------------\n 1 file changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/kwset.c b/kwset.c\nindex 1714eada608..064329434e5 100644\n--- a/kwset.c\n+++ b/kwset.c\n@@ -197,10 +197,13 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n while (link && label != link->label)\n \t{\n \t links[depth] = link;\n-\t if (label < link->label)\n-\t dirs[depth++] = L, link = link->llink;\n-\t else\n-\t dirs[depth++] = R, link = link->rlink;\n+\t if (label < link->label) {\n+\t dirs[depth++] = L;\n+\t link = link->llink;\n+\t } else {\n+\t dirs[depth++] = R;\n+\t link = link->rlink;\n+\t }\n \t}\n \n /* The current character doesn't have an outgoing link at\n@@ -257,14 +260,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n \t\t switch (dirs[depth + 1])\n \t\t {\n \t\t case L:\n-\t\t r = links[depth], t = r->llink, rl = t->rlink;\n-\t\t t->rlink = r, r->llink = rl;\n+\t\t r = links[depth]; t = r->llink; rl = t->rlink;\n+\t\t t->rlink = r; r->llink = rl;\n \t\t t->balance = r->balance = 0;\n \t\t break;\n \t\t case R:\n-\t\t r = links[depth], l = r->llink, t = l->rlink;\n-\t\t rl = t->rlink, lr = t->llink;\n-\t\t t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;\n+\t\t r = links[depth]; l = r->llink; t = l->rlink;\n+\t\t rl = t->rlink; lr = t->llink;\n+\t\t t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;\n \t\t l->balance = t->balance != 1 ? 0 : -1;\n \t\t r->balance = t->balance != (char) -1 ? 0 : 1;\n \t\t t->balance = 0;\n@@ -277,14 +280,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n \t\t switch (dirs[depth + 1])\n \t\t {\n \t\t case R:\n-\t\t l = links[depth], t = l->rlink, lr = t->llink;\n-\t\t t->llink = l, l->rlink = lr;\n+\t\t l = links[depth]; t = l->rlink; lr = t->llink;\n+\t\t t->llink = l; l->rlink = lr;\n \t\t t->balance = l->balance = 0;\n \t\t break;\n \t\t case L:\n-\t\t l = links[depth], r = l->rlink, t = r->llink;\n-\t\t lr = t->llink, rl = t->rlink;\n-\t\t t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;\n+\t\t l = links[depth]; r = l->rlink; t = r->llink;\n+\t\t lr = t->llink; rl = t->rlink;\n+\t\t t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;\n \t\t l->balance = t->balance != 1 ? 0 : -1;\n \t\t r->balance = t->balance != (char) -1 ? 0 : 1;\n \t\t t->balance = 0;\n@@ -567,22 +570,22 @@ bmexec (kwset_t kws, char const *text, size_t size)\n {\n \twhile (tp <= ep)\n \t {\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t }\n \tbreak;\n found:\n@@ -649,7 +652,8 @@ cwexec (kwset_t kws, char const *text, size_t len, struct kwsmatch *kwsmatch)\n mch = NULL;\n else\n {\n- mch = text, accept = kwset->trie;\n+ mch = text;\n+ accept = kwset->trie;\n goto match;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"515056","messageId":"f60ebe376e10d7741f6bd657874a17f6c09d4477.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 04/10] clar: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:08Z","receivedAt":"2025-03-25T23:32:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. In this instance, it\nmakes the code harder to read than necessary, too. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n t/unit-tests/clar/clar/fs.h | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/unit-tests/clar/clar/fs.h b/t/unit-tests/clar/clar/fs.h\nindex 8b206179fc4..2203743fb48 100644\n--- a/t/unit-tests/clar/clar/fs.h\n+++ b/t/unit-tests/clar/clar/fs.h\n@@ -376,9 +376,12 @@ fs_copydir_helper(const char *source, const char *dest, int dest_mode)\n \tmkdir(dest, dest_mode);\n \n \tcl_assert_(source_dir = opendir(source), \"Could not open source dir\");\n-\twhile ((d = (errno = 0, readdir(source_dir))) != NULL) {\n+\tfor (;;) {\n \t\tchar *child;\n \n+\t\terrno = 0;\n+\t\tif ((d = readdir(source_dir)) == NULL)\n+\t\t\tbreak;\n \t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n \t\t\tcontinue;\n \n@@ -479,9 +482,12 @@ fs_rmdir_helper(const char *path)\n \tstruct dirent *d;\n \n \tcl_assert_(dir = opendir(path), \"Could not open dir\");\n-\twhile ((d = (errno = 0, readdir(dir))) != NULL) {\n+\tfor (;;) {\n \t\tchar *child;\n \n+\t\terrno = 0;\n+\t\tif ((d = readdir(dir)) == NULL)\n+\t\t\tbreak;\n \t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n \t\t\tcontinue;\n \n-- \ngitgitgadget\n\n"},{"id":"515057","messageId":"7239078413f6a223105939af1b56e79b9d302f1f.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 05/10] xdiff: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:09Z","receivedAt":"2025-03-25T23:32:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. While the code in\nthis patch used the comma operator intentionally (to avoid curly\nbrackets around two statements, each, that want to be guarded by a\ncondition), it is better to surround it with curly brackets and to use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n xdiff/xdiffi.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 8889b8b62a1..5a96e36dfbe 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -211,8 +211,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n \t\t\tfor (d = fmax; d >= fmin; d -= 2) {\n \t\t\t\ti1 = XDL_MIN(kvdf[d], lim1);\n \t\t\t\ti2 = i1 - d;\n-\t\t\t\tif (lim2 < i2)\n-\t\t\t\t\ti1 = lim2 + d, i2 = lim2;\n+\t\t\t\tif (lim2 < i2) {\n+\t\t\t\t\ti1 = lim2 + d;\n+\t\t\t\t\ti2 = lim2;\n+\t\t\t\t}\n \t\t\t\tif (fbest < i1 + i2) {\n \t\t\t\t\tfbest = i1 + i2;\n \t\t\t\t\tfbest1 = i1;\n@@ -223,8 +225,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n \t\t\tfor (d = bmax; d >= bmin; d -= 2) {\n \t\t\t\ti1 = XDL_MAX(off1, kvdb[d]);\n \t\t\t\ti2 = i1 - d;\n-\t\t\t\tif (i2 < off2)\n-\t\t\t\t\ti1 = off2 + d, i2 = off2;\n+\t\t\t\tif (i2 < off2) {\n+\t\t\t\t\ti1 = off2 + d;\n+\t\t\t\t\ti2 = off2;\n+\t\t\t\t}\n \t\t\t\tif (i1 + i2 < bbest) {\n \t\t\t\t\tbbest = i1 + i2;\n \t\t\t\t\tbbest1 = i1;\n-- \ngitgitgadget\n\n"},{"id":"515058","messageId":"5e0e8325620395196608a0851f1f6fc9408f6090.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:10Z","receivedAt":"2025-03-25T23:32:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. That is why the\n`-Wcomma` option of clang was introduced: To identify unintentional uses\nof the comma operator.\n\nIntentional uses include situations where one wants to avoid curly\nbrackets around multiple statements that need to be guarded by a\ncondition. This is the case here, as the repetitive nature of the\nstatements is easier to see for a human reader this way.\n\nTo mark this usage as intentional, the return value of the statement\nbefore the comma needs to be cast to `void`, which we do here.\n\nSigned-off-by: Johannes Schindelin \n---\n diff-delta.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex a4faf73829b..a03ba10b2be 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -439,18 +439,18 @@ create_delta(const struct delta_index *index,\n \t\t\ti = 0x80;\n \n \t\t\tif (moff & 0x000000ff)\n-\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n+\t\t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n \t\t\tif (moff & 0x0000ff00)\n-\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n+\t\t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n \t\t\tif (moff & 0x00ff0000)\n-\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n+\t\t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n \t\t\tif (moff & 0xff000000)\n-\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n+\t\t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n \n \t\t\tif (msize & 0x00ff)\n-\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n+\t\t\t\t(void)(out[outpos++] = msize >> 0), i |= 0x10;\n \t\t\tif (msize & 0xff00)\n-\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n+\t\t\t\t(void)(out[outpos++] = msize >> 8), i |= 0x20;\n \n \t\t\t*op = i;\n \n-- \ngitgitgadget\n\n"},{"id":"515059","messageId":"9a6de12b8076266fb0c88f6b658c20d37409ce13.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:11Z","receivedAt":"2025-03-25T23:32:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. That is why the\n`-Wcomma` option of clang was introduced: To identify unintentional uses\nof the comma operator.\n\nTo mark such a usage as intentional, the value needs to be cast to\n`void`, which we do here.\n\nIn this instance, the usage is intentional because it allows storing the\nvalue of the current character as `prev_ch` before making the next\ncharacter the current one, all of which happens in the loop condition\nthat lets the loop stop at a closing bracket.\n\nThe alternative to using the comma operator would be to move those\nassignments from the condition into the loop body; In this particular\ncase that would require the assignments to either be duplicated or to\nintroduce and use a `goto` target before the assignments, though,\nbecause the loop body contains a `continue` for the case where a\ncharacter class is found that starts with `[:` but does not end in `:]`\n(and the assignments should occur even when that code path is taken).\n\nSigned-off-by: Johannes Schindelin \n---\n wildmatch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 8ea29141bd7..ce8108a6d57 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n \t\t\t\t} else if (t_ch == p_ch)\n \t\t\t\t\tmatched = 1;\n-\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n+\t\t\t} while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');\n \t\t\tif (matched == negated ||\n \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n \t\t\t\treturn WM_NOMATCH;\n-- \ngitgitgadget\n\n"},{"id":"515060","messageId":"dc626f36df34df4897289e508dbf608512a93870.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:12Z","receivedAt":"2025-03-25T23:32:25Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. That is why the\n`-Wcomma` option of clang was introduced: To identify unintentional uses\nof the comma operator.\n\nIn the `compat/regex/` code, the comma operator is used twice, once to\navoid surrounding two conditional statements with curly brackets, the\nother one to increment two counters simultaneously in a `do ... while`\ncondition.\n\nThe first one is replaced with a proper conditional block, surrounded by\ncurly brackets.\n\nThe second one would be harder to replace because the loop contains two\n`continue`s. Therefore, the second one is marked as intentional by\ncasting the value-to-discard to `void`.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/regex/regex_internal.c | 7 ++++---\n compat/regex/regexec.c | 2 +-\n 2 files changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c\nindex ec5cc5d2dd1..7672583bf7e 100644\n--- a/compat/regex/regex_internal.c\n+++ b/compat/regex/regex_internal.c\n@@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)\n for (sbase = dest->nelem + 2 * src->nelem,\n is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )\n {\n- if (dest->elems[id] == src->elems[is])\n-\tis--, id--;\n- else if (dest->elems[id] < src->elems[is])\n+ if (dest->elems[id] == src->elems[is]) {\n+\tis--;\n+\tid--;\n+ } else if (dest->elems[id] < src->elems[is])\n \tdest->elems[--sbase] = src->elems[is--];\n else /* if (dest->elems[id] > src->elems[is]) */\n \t--id;\ndiff --git a/compat/regex/regexec.c b/compat/regex/regexec.c\nindex 2eeec82f407..c08f1bbe1f5 100644\n--- a/compat/regex/regexec.c\n+++ b/compat/regex/regexec.c\n@@ -2210,7 +2210,7 @@ sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,\n \t /* mctx->bkref_ents may have changed, reload the pointer. */\n \t entry = mctx->bkref_ents + enabled_idx;\n \t}\n- while (enabled_idx++, entry++->more);\n+ while ((void)enabled_idx++, entry++->more);\n }\n err = REG_NOERROR;\n free_return:\n-- \ngitgitgadget\n\n"},{"id":"515061","messageId":"91f86c3aba9d19d5df11661675fd6c2cc049e191.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 09/10] clang: warn when the comma operator is used","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:13Z","receivedAt":"2025-03-25T23:32:26Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen compiling Git using `clang`, the `-Wcomma` option can be used to\nwarn about code using the comma operator (because it is typically\nunintentional and wants to use the semicolon instead).\n\nSigned-off-by: Johannes Schindelin \n---\n config.mak.dev | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 0fd8cc4d355..31423638169 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla\n DEVELOPER_CFLAGS += -Wwrite-strings\n DEVELOPER_CFLAGS += -fno-common\n \n+ifneq ($(filter clang9,$(COMPILER_FEATURES)),)\n+DEVELOPER_CFLAGS += -Wcomma\n+endif\n+\n ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n endif\n-- \ngitgitgadget\n\n"},{"id":"515062","messageId":"2f6f31240fe6ce5f8efab662af477540a0f966ca.1742945534.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-25T23:32:14Z","receivedAt":"2025-03-25T23:32:27Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn my setup, clang finds `/usr/local/cuda` and hence the output of\n`clang -v` ends with this line:\n\n\tFound CUDA installation: /usr/local/cuda, version\n\nThis confuses the `detect-compiler` script because it matches _all_\nlines that contain the needle \"version\" surrounded by spaces. As a\nconsequence, the `get_family` function returns two lines: \"Ubuntu clang\"\nand above-mentioned line, which the `case` statement does not handle\nwell and hence reports \"unknown compiler family\" instead of the expected\nset of \"clang14\", \"clang13\", ..., \"clang1\" output.\n\nLet's unconfuse the script by letting it parse the first matching line\nand ignore the rest.\n\nSigned-off-by: Johannes Schindelin \n---\n detect-compiler | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/detect-compiler b/detect-compiler\nindex a87650b71bb..01eca3a781d 100755\n--- a/detect-compiler\n+++ b/detect-compiler\n@@ -9,7 +9,7 @@ CC=\"$*\"\n #\n # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n get_version_line() {\n-\tLANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n+\tLANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'\n }\n \n get_family() {\n-- \ngitgitgadget\n"},{"id":"515070","messageId":"Z-OWmSe3vCDaZLXy@pks.im","threadId":"63188","inReplyTo":"f60ebe376e10d7741f6bd657874a17f6c09d4477.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 04/10] clar: avoid using the comma operator unnecessarily","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-26T05:54:33Z","receivedAt":"2025-03-26T05:54:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:08PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The comma operator is a somewhat obscure C feature that is often used by\n> mistake and can even cause unintentional code flow. In this instance, it\n> makes the code harder to read than necessary, too. Better use a\n> semicolon instead.\n\nThis code has changed upstream already, but let's roll with your change\nanyway. I plan to update the clar to the upstream version soonish once I\nhave landed integer comparisons, and will take care that there aren't\nany other comment operators left.\n\nPatrick\n"},{"id":"515071","messageId":"Z-OWoK-DlvnxXPkO@pks.im","threadId":"63188","inReplyTo":"5e0e8325620395196608a0851f1f6fc9408f6090.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-26T05:54:40Z","receivedAt":"2025-03-26T05:54:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:10PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The comma operator is a somewhat obscure C feature that is often used by\n> mistake and can even cause unintentional code flow. That is why the\n> `-Wcomma` option of clang was introduced: To identify unintentional uses\n> of the comma operator.\n> \n> Intentional uses include situations where one wants to avoid curly\n> brackets around multiple statements that need to be guarded by a\n> condition. This is the case here, as the repetitive nature of the\n> statements is easier to see for a human reader this way.\n> \n> To mark this usage as intentional, the return value of the statement\n> before the comma needs to be cast to `void`, which we do here.\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> diff-delta.c | 12 ++++++------\n> 1 file changed, 6 insertions(+), 6 deletions(-)\n> \n> diff --git a/diff-delta.c b/diff-delta.c\n> index a4faf73829b..a03ba10b2be 100644\n> --- a/diff-delta.c\n> +++ b/diff-delta.c\n> @@ -439,18 +439,18 @@ create_delta(const struct delta_index *index,\n> \t\t\ti = 0x80;\n> \n> \t\t\tif (moff & 0x000000ff)\n> -\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n> +\t\t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n> \t\t\tif (moff & 0x0000ff00)\n> -\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n> +\t\t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n> \t\t\tif (moff & 0x00ff0000)\n> -\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n> +\t\t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n> \t\t\tif (moff & 0xff000000)\n> -\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n> +\t\t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n> \n> \t\t\tif (msize & 0x00ff)\n> -\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n> +\t\t\t\t(void)(out[outpos++] = msize >> 0), i |= 0x10;\n> \t\t\tif (msize & 0xff00)\n> -\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n> +\t\t\t\t(void)(out[outpos++] = msize >> 8), i |= 0x20;\n\nHm. I think the end result is even more confusing than before. Why don't\nwe introduce curly braces here, same as in preceding commits?\n\nPatrick\n"},{"id":"515072","messageId":"Z-OWpEDwHtaq3r2J@pks.im","threadId":"63188","inReplyTo":"9a6de12b8076266fb0c88f6b658c20d37409ce13.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-26T05:54:44Z","receivedAt":"2025-03-26T05:54:47Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:11PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/wildmatch.c b/wildmatch.c\n> index 8ea29141bd7..ce8108a6d57 100644\n> --- a/wildmatch.c\n> +++ b/wildmatch.c\n> @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n> \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n> \t\t\t\t} else if (t_ch == p_ch)\n> \t\t\t\t\tmatched = 1;\n> -\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n> +\t\t\t} while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');\n> \t\t\tif (matched == negated ||\n> \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n> \t\t\t\treturn WM_NOMATCH;\n\nIn this case I agree that it makes sense to not introduce curly braces\nfor brevity.\n\nPatrick\n"},{"id":"515073","messageId":"Z-OWp0NkjrMVQieH@pks.im","threadId":"63188","inReplyTo":"91f86c3aba9d19d5df11661675fd6c2cc049e191.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 09/10] clang: warn when the comma operator is used","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-26T05:54:47Z","receivedAt":"2025-03-26T05:54:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:13PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> When compiling Git using `clang`, the `-Wcomma` option can be used to\n> warn about code using the comma operator (because it is typically\n> unintentional and wants to use the semicolon instead).\n> \n> Signed-off-by: Johannes Schindelin \n> ---\n> config.mak.dev | 4 ++++\n> 1 file changed, 4 insertions(+)\n> \n> diff --git a/config.mak.dev b/config.mak.dev\n> index 0fd8cc4d355..31423638169 100644\n> --- a/config.mak.dev\n> +++ b/config.mak.dev\n> @@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla\n> DEVELOPER_CFLAGS += -Wwrite-strings\n> DEVELOPER_CFLAGS += -fno-common\n> \n> +ifneq ($(filter clang9,$(COMPILER_FEATURES)),)\n> +DEVELOPER_CFLAGS += -Wcomma\n> +endif\n> +\n> ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n> DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n> endif\n\nLet's squash the below diff into this commit. The loop already makes\nsure that the compiler supports the flag, so there is no need to special\ncase Clang.\n\nPatrick\n\ndiff --git a/meson.build b/meson.build\nindex dd231b669b6..a7658d62ea0 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -717,6 +717,7 @@ libgit_dependencies = [ ]\n # Makefile.\n if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'\n foreach cflag : [\n+ '-Wcomma',\n '-Wdeclaration-after-statement',\n '-Wformat-security',\n '-Wold-style-definition',\n"},{"id":"515074","messageId":"Z-OWqs33rcgbYcR8@pks.im","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/10] Avoid the comma operator","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-26T05:54:50Z","receivedAt":"2025-03-26T05:54:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> The comma operator\n> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> rarely used in C anymore, and typically indicates a typo. Just like in these\n> instances, where a semicolon was meant to be used, as there is no need to\n> discard the first statement's result here.\n> \n> Changes since v1:\n> \n> * Use -Wcomma when compiling with clang and with DEVELOPER=1.\n> * Address the remaining instances pointed out by clang (and by Phillip).\n\nThanks for all of these fixes!\n\nPatrick\n"},{"id":"515076","messageId":"b6b1fdc7-2ec3-d569-fc58-5f238e98f428@gmx.de","threadId":"63188","inReplyTo":"Z-OWmSe3vCDaZLXy@pks.im","subject":"Re: [PATCH v2 04/10] clar: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-26T07:03:36Z","receivedAt":"2025-03-26T07:03:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n\n> On Tue, Mar 25, 2025 at 11:32:08PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > The comma operator is a somewhat obscure C feature that is often used by\n> > mistake and can even cause unintentional code flow. In this instance, it\n> > makes the code harder to read than necessary, too. Better use a\n> > semicolon instead.\n>\n> This code has changed upstream already, but let's roll with your change\n> anyway. I plan to update the clar to the upstream version soonish once I\n> have landed integer comparisons, and will take care that there aren't\n> any other comment operators left.\n\nThank you for putting so much energy into improving the tests, I really\nappreciate it!\n\nCiao,\nJohannes\n"},{"id":"515078","messageId":"050f5d65-32eb-fd26-1461-579b06018dc6@gmx.de","threadId":"63188","inReplyTo":"Z-OWoK-DlvnxXPkO@pks.im","subject":"Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-26T07:20:32Z","receivedAt":"2025-03-26T07:20:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n\n> On Tue, Mar 25, 2025 at 11:32:10PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > The comma operator is a somewhat obscure C feature that is often used by\n> > mistake and can even cause unintentional code flow. That is why the\n> > `-Wcomma` option of clang was introduced: To identify unintentional uses\n> > of the comma operator.\n> >\n> > Intentional uses include situations where one wants to avoid curly\n> > brackets around multiple statements that need to be guarded by a\n> > condition. This is the case here, as the repetitive nature of the\n> > statements is easier to see for a human reader this way.\n> >\n> > To mark this usage as intentional, the return value of the statement\n> > before the comma needs to be cast to `void`, which we do here.\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > diff-delta.c | 12 ++++++------\n> > 1 file changed, 6 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/diff-delta.c b/diff-delta.c\n> > index a4faf73829b..a03ba10b2be 100644\n> > --- a/diff-delta.c\n> > +++ b/diff-delta.c\n> > @@ -439,18 +439,18 @@ create_delta(const struct delta_index *index,\n> > \t\t\ti = 0x80;\n> >\n> > \t\t\tif (moff & 0x000000ff)\n> > -\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n> > +\t\t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n> > \t\t\tif (moff & 0x0000ff00)\n> > -\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n> > +\t\t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n> > \t\t\tif (moff & 0x00ff0000)\n> > -\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n> > +\t\t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n> > \t\t\tif (moff & 0xff000000)\n> > -\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n> > +\t\t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n> >\n> > \t\t\tif (msize & 0x00ff)\n> > -\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n> > +\t\t\t\t(void)(out[outpos++] = msize >> 0), i |= 0x10;\n> > \t\t\tif (msize & 0xff00)\n> > -\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n> > +\t\t\t\t(void)(out[outpos++] = msize >> 8), i |= 0x20;\n>\n> Hm. I think the end result is even more confusing than before. Why don't\n> we introduce curly braces here, same as in preceding commits?\n\nThe interleaved -/+ lines make it admittedly hard to see what I meant.\nI'll unwind it a bit (presenting only the `moff` part, the same\nconsideration applies to `msize`):\n\n\t\tif (moff & 0x000000ff)\n\t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n\t\tif (moff & 0x0000ff00)\n\t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n\t\tif (moff & 0x00ff0000)\n\t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n\t\tif (moff & 0xff000000)\n\t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n\nIn this form, it is very obvious (from comparing the right-side half of\nthe lines) that a shifted version of `moff` is appended to `out` and `i`\ngets a bit set, and the correlation between shift width and the set bit\nis relatively easy to see and validate (at least my brain has an easy time\nhere, thanks to the alignment and thanks to visual similarity between the\nnon-blank parts).\n\nIt is admittedly quite a bit harder not to be distracted by the repetitive\n`(void)(out[...` parts to understand and validate the `if` conditions on\nthe left-hand side, but thanks to those repetitive parts being identical,\nand being only one line between those `if` lines, I can bring my brain to\nfocus only on the differences of the bitmask and understand and verify\nthem with relatively little effort.\n\nWhen I compared this form to the following, the cognitive load to wrap my\nhead around the code is quite a bit higher there:\n\n\t\tif (moff & 0x000000ff) {\n\t\t\tout[outpos++] = moff >> 0;\n\t\t\ti |= 0x01;\n\t\t}\n\t\tif (moff & 0x0000ff00) {\n\t\t\tout[outpos++] = moff >> 8;\n\t\t\ti |= 0x02;\n\t\t}\n\t\tif (moff & 0x00ff0000) {\n\t\t\tout[outpos++] = moff >> 16;\n\t\t\ti |= 0x04;\n\t\t}\n\t\tif (moff & 0xff000000) {\n\t\t\tout[outpos++] = moff >> 24;\n\t\t\ti |= 0x08;\n\t\t}\n\nThe reason is the visual distance between the near-identical code.\n\nHaving said that, I do realize that my brain quite possibly works in\nspecial ways here and that the general preference is to go with the latter\nform.\n\nDo you have a strong opinion which form to use?\n\nCiao,\nJohannes\n"},{"id":"515079","messageId":"721cfcca-b702-cb44-c847-f438b2495bda@gmx.de","threadId":"63188","inReplyTo":"Z-OWpEDwHtaq3r2J@pks.im","subject":"Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-26T07:46:29Z","receivedAt":"2025-03-26T07:46:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n\n> On Tue, Mar 25, 2025 at 11:32:11PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > diff --git a/wildmatch.c b/wildmatch.c\n> > index 8ea29141bd7..ce8108a6d57 100644\n> > --- a/wildmatch.c\n> > +++ b/wildmatch.c\n> > @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n> > \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n> > \t\t\t\t} else if (t_ch == p_ch)\n> > \t\t\t\t\tmatched = 1;\n> > -\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n> > +\t\t\t} while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');\n> > \t\t\tif (matched == negated ||\n> > \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n> > \t\t\t\treturn WM_NOMATCH;\n>\n> In this case I agree that it makes sense to not introduce curly braces\n> for brevity.\n\nI should probably have mentioned that this patch took the longest to write\nof the entire patch series, by far. Not because of the changed code, that\nwas easy. No, when I wrote the commit message and spotted the `continue`,\nI did not want to leave it at that because the code around that `continue`\nlooks... well, let's just say that it could be rewritten for clarity.\n\nIn fact, when I looked at the following part, I was immediately\n_convinced_ that it is incorrect, and had to work very hard to understand\nwhy it works correctly, even going so far as to single-step through a\ncouple of examples, e.g. `test-tool wildmatch wildmatch 'b' '[[:a-z]'`:\n\n\t\t\t\t} else if (p_ch == '[' && p[1] == ':') {\n\t\t\t\t\tconst uchar *s;\n\t\t\t\t\tint i;\n\t\t\t\t\tfor (s = p += 2; (p_ch = *p) && p_ch != ']'; p++) {} /*SHARED ITERATOR*/\n\t\t\t\t\tif (!p_ch)\n\t\t\t\t\t\treturn WM_ABORT_ALL;\n\t\t\t\t\ti = p - s - 1;\n\t\t\t\t\tif (i < 0 || p[-1] != ':') {\n\t\t\t\t\t\t/* Didn't find \":]\", so treat like a normal set. */\n\t\t\t\t\t\tp = s - 2;\n\t\t\t\t\t\tp_ch = '[';\n\t\t\t\t\t\tif (t_ch == p_ch)\n\t\t\t\t\t\t\tmatched = 1;\n\t\t\t\t\t\tcontinue;\n\t\t\t\t\t}\n\nFor context, here is a link:\nhttps://gitlab.com/gitlab-org/git/-/blob/v2.49.0/wildmatch.c?ref_type=tags#L213-227.\nAt this point, `t_ch` is the current character in the text to match;\n`p_ch` (and `*p`) is the current character in the _pattern_, and it is\n_inside_ a `[...]` character range, and it wants to match a character\nclass of the form `[:alnum:]` but also allow for a regular character range\nthat starts by including the colon. And that latter scenario, where it is\n_not_ a special character class, is what this `continue` is all about.\n\nWhat threw me was that `t_ch == p_ch` condition _directly_ after assigning\n`p_ch = '['`. It is still a pattern I would always immediately suspect to\nbe a bug: Why not compare `t_ch == '['` instead, which would be much more\nobvious?\n\nYou will also note that the value of `i` is only used in the condition,\nand it is basically used to determine whether the the colon was\nimmediately followed by the closing bracket or not, which could be\nrewritten to be a lot more obvious.\n\nSo what does the `continue` do here? It skips back to the outer loop,\ncontinuing with the `:` as next pattern character in the character range,\nand for that it is crucial that the `p_ch` be set to the opening bracket\nand `p` is rewound _just_ so that the assignments in the loop condition\ncan set things up for the next loop iteration (still within that `case\n'['`) not to be thrown by that `[:`.\n\nI probably did a terrible job explaining why the code works as intended,\nbut I'll just chalk it up to the contortions my brain had to exercise to\nunderstand that code.\n\nBut all that's safely outside the scope of the question whether to use a\ncomma operator or not ;-)\n\nCiao,\nJohannes\n"},{"id":"515080","messageId":"xmqq4izgjknz.fsf@gitster.g","threadId":"63188","inReplyTo":"9a6de12b8076266fb0c88f6b658c20d37409ce13.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-26T07:49:52Z","receivedAt":"2025-03-26T07:49:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> diff --git a/wildmatch.c b/wildmatch.c\n> index 8ea29141bd7..ce8108a6d57 100644\n> --- a/wildmatch.c\n> +++ b/wildmatch.c\n> @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n> \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n> \t\t\t\t} else if (t_ch == p_ch)\n> \t\t\t\t\tmatched = 1;\n> -\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n> +\t\t\t} while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');\n> \t\t\tif (matched == negated ||\n> \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n> \t\t\t\treturn WM_NOMATCH;\n\nHmph, I personally do not find the (void) convention any easier to\nread and understand than the original, and more importantly, it does\nnot look like a good signal to say that the author knows what they\nare doing.\n\nFor this particular loop, it probably makes a lot more sense to do a\nstupid and more obvious rewrite. And the same thing can be said for\nthe previous step.\n\nPerhaps like so?\n\n\n diff-delta.c | 37 ++++++++++++++++++++++++-------------\n wildmatch.c | 4 +++-\n 2 files changed, 27 insertions(+), 14 deletions(-)\n\ndiff --git c/diff-delta.c w/diff-delta.c\nindex a4faf73829..a999b875d4 100644\n--- c/diff-delta.c\n+++ w/diff-delta.c\n@@ -438,19 +438,30 @@ create_delta(const struct delta_index *index,\n \t\t\top = out + outpos++;\n \t\t\ti = 0x80;\n \n-\t\t\tif (moff & 0x000000ff)\n-\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n-\t\t\tif (moff & 0x0000ff00)\n-\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n-\t\t\tif (moff & 0x00ff0000)\n-\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n-\t\t\tif (moff & 0xff000000)\n-\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n-\n-\t\t\tif (msize & 0x00ff)\n-\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n-\t\t\tif (msize & 0xff00)\n-\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n+\t\t\tif (moff & 0x000000ff) {\n+\t\t\t\tout[outpos++] = moff >> 0;\n+\t\t\t\ti |= 0x01;\n+\t\t\t}\n+\t\t\tif (moff & 0x0000ff00) {\n+\t\t\t\tout[outpos++] = moff >> 8;\n+\t\t\t\ti |= 0x02;\n+\t\t\t}\n+\t\t\tif (moff & 0x00ff0000) {\n+\t\t\t\tout[outpos++] = moff >> 16;\n+\t\t\t\ti |= 0x04;\n+\t\t\t}\n+\t\t\tif (moff & 0xff000000) {\n+\t\t\t\tout[outpos++] = moff >> 24;\n+\t\t\t\ti |= 0x08;\n+\t\t\t}\n+\t\t\tif (msize & 0x00ff) {\n+\t\t\t\tout[outpos++] = msize >> 0;\n+\t\t\t\ti |= 0x10;\n+\t\t\t}\n+\t\t\tif (msize & 0xff00) {\n+\t\t\t\tout[outpos++] = msize >> 8;\n+\t\t\t\ti |= 0x20;\n+\t\t\t}\n \n \t\t\t*op = i;\n \ndiff --git c/wildmatch.c w/wildmatch.c\nindex 8ea29141bd..29f4b4df75 100644\n--- c/wildmatch.c\n+++ w/wildmatch.c\n@@ -268,7 +268,9 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n \t\t\t\t} else if (t_ch == p_ch)\n \t\t\t\t\tmatched = 1;\n-\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n+\n+\t\t\t\tprev_ch = p_ch;\n+\t\t\t} while ((p_ch = *++p) != ']');\n \t\t\tif (matched == negated ||\n \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n \t\t\t\treturn WM_NOMATCH;\n\n\n"},{"id":"515081","messageId":"9cd9a690-1258-d96d-33c9-53a3b0f346a1@gmx.de","threadId":"63188","inReplyTo":"Z-OWp0NkjrMVQieH@pks.im","subject":"Re: [PATCH v2 09/10] clang: warn when the comma operator is used","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-26T07:50:24Z","receivedAt":"2025-03-26T07:50:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n\n> On Tue, Mar 25, 2025 at 11:32:13PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > From: Johannes Schindelin \n> >\n> > When compiling Git using `clang`, the `-Wcomma` option can be used to\n> > warn about code using the comma operator (because it is typically\n> > unintentional and wants to use the semicolon instead).\n> >\n> > Signed-off-by: Johannes Schindelin \n> > ---\n> > config.mak.dev | 4 ++++\n> > 1 file changed, 4 insertions(+)\n> >\n> > diff --git a/config.mak.dev b/config.mak.dev\n> > index 0fd8cc4d355..31423638169 100644\n> > --- a/config.mak.dev\n> > +++ b/config.mak.dev\n> > @@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla\n> > DEVELOPER_CFLAGS += -Wwrite-strings\n> > DEVELOPER_CFLAGS += -fno-common\n> >\n> > +ifneq ($(filter clang9,$(COMPILER_FEATURES)),)\n> > +DEVELOPER_CFLAGS += -Wcomma\n> > +endif\n> > +\n> > ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n> > DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n> > endif\n>\n> Let's squash the below diff into this commit. The loop already makes\n> sure that the compiler supports the flag, so there is no need to special\n> case Clang.\n\nOkay, will do.\n\nFor my curiosity...\n\n> diff --git a/meson.build b/meson.build\n> index dd231b669b6..a7658d62ea0 100644\n> --- a/meson.build\n> +++ b/meson.build\n> @@ -717,6 +717,7 @@ libgit_dependencies = [ ]\n> # Makefile.\n> if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'\n\nThis `get_argument_syntax() == 'gcc'` condition catches `clang`? What\nother compilers that aren't GCC does it catch?\n\nCiao,\nJohannes\n\n> foreach cflag : [\n> + '-Wcomma',\n> '-Wdeclaration-after-statement',\n> '-Wformat-security',\n> '-Wold-style-definition',\n>\n"},{"id":"515087","messageId":"Z-O79qs_iL0lYany@pks.im","threadId":"63188","inReplyTo":"9cd9a690-1258-d96d-33c9-53a3b0f346a1@gmx.de","subject":"Re: [PATCH v2 09/10] clang: warn when the comma operator is used","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-03-26T08:33:58Z","receivedAt":"2025-03-26T08:34:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Mar 26, 2025 at 08:50:24AM +0100, Johannes Schindelin wrote:\n> On Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n> > diff --git a/meson.build b/meson.build\n> > index dd231b669b6..a7658d62ea0 100644\n> > --- a/meson.build\n> > +++ b/meson.build\n> > @@ -717,6 +717,7 @@ libgit_dependencies = [ ]\n> > # Makefile.\n> > if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'\n> \n> This `get_argument_syntax() == 'gcc'` condition catches `clang`? What\n> other compilers that aren't GCC does it catch?\n\nYes, it does catch Clang as well as the Intel compiler. The list of\ncompilers and their respective syntax can be found at [1]\n\nPatrick\n\n[1]: https://mesonbuild.com/Reference-tables.html#compiler-ids\n"},{"id":"515090","messageId":"a7506e7b-f86f-426c-b816-540be282518b@gmail.com","threadId":"63188","inReplyTo":"9a6de12b8076266fb0c88f6b658c20d37409ce13.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-26T10:14:32Z","receivedAt":"2025-03-26T10:14:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 25/03/2025 23:32, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> The alternative to using the comma operator would be to move those\n> assignments from the condition into the loop body; In this particular\n> case that would require the assignments to either be duplicated or to\n> introduce and use a `goto` target before the assignments, though,\n> because the loop body contains a `continue` for the case where a\n> character class is found that starts with `[:` but does not end in `:]`\n> (and the assignments should occur even when that code path is taken).\n\nI think that would be clearer, something like the diff below\n\nBest Wishes\n\nPhillip\n\n---- >8 ----\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 8ea29141bd7..7230544c356 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -223,7 +223,7 @@\n \t\t\t\t\t\tp_ch = '[';\n \t\t\t\t\t\tif (t_ch == p_ch)\n \t\t\t\t\t\t\tmatched = 1;\n-\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t\tgoto next;\n \t\t\t\t\t}\n \t\t\t\t\tif (CC_EQ(s,i, \"alnum\")) {\n \t\t\t\t\t\tif (ISALNUM(t_ch))\n@@ -268,7 +268,10 @@\n \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n \t\t\t\t} else if (t_ch == p_ch)\n \t\t\t\t\tmatched = 1;\n-\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n+\t\t\tnext:\n+\t\t\t\tprev_ch = p_ch;\n+\t\t\t\tp_ch = *++p;\n+\t\t\t} while (p_ch != ']');\n \t\t\tif (matched == negated ||\n \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n \t\t\t\treturn WM_NOMATCH;\n\n\n\nPhillip> Signed-off-by: Johannes Schindelin \n> ---\n> wildmatch.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/wildmatch.c b/wildmatch.c\n> index 8ea29141bd7..ce8108a6d57 100644\n> --- a/wildmatch.c\n> +++ b/wildmatch.c\n> @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n> \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n> \t\t\t\t} else if (t_ch == p_ch)\n> \t\t\t\t\tmatched = 1;\n> -\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n> +\t\t\t} while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');\n> \t\t\tif (matched == negated ||\n> \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n> \t\t\t\treturn WM_NOMATCH;\n\n"},{"id":"515091","messageId":"ad57203b-8925-4f88-add6-763c2d80afac@gmail.com","threadId":"63188","inReplyTo":"050f5d65-32eb-fd26-1461-579b06018dc6@gmx.de","subject":"Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-26T10:17:50Z","receivedAt":"2025-03-26T10:17:53Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 26/03/2025 07:20, Johannes Schindelin wrote:\n> Hi Patrick,\n> On Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n>> Hm. I think the end result is even more confusing than before. Why don't\n>> we introduce curly braces here, same as in preceding commits?\n> \n> The interleaved -/+ lines make it admittedly hard to see what I meant.\n> I'll unwind it a bit (presenting only the `moff` part, the same\n> consideration applies to `msize`):\n> \n> \t\tif (moff & 0x000000ff)\n> \t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n> \t\tif (moff & 0x0000ff00)\n> \t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n> \t\tif (moff & 0x00ff0000)\n> \t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n> \t\tif (moff & 0xff000000)\n> \t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n> \n> In this form, it is very obvious (from comparing the right-side half of\n> the lines) that a shifted version of `moff` is appended to `out` and `i`\n> gets a bit set, and the correlation between shift width and the set bit\n> is relatively easy to see and validate (at least my brain has an easy time\n> here, thanks to the alignment and thanks to visual similarity between the\n> non-blank parts).\n> \n> It is admittedly quite a bit harder not to be distracted by the repetitive\n> `(void)(out[...` parts to understand and validate the `if` conditions on\n> the left-hand side,\n\nThat makes it pretty unreadable for me. If you're worried about the \nvertical space we could perhaps keep both statements on a single line so \nthat we're only adding a single newline for the closing brace rather \nthan two.\n\nBest Wishes\n\nPhillip\n\n but thanks to those repetitive parts being identical,\n> and being only one line between those `if` lines, I can bring my brain to\n> focus only on the differences of the bitmask and understand and verify\n> them with relatively little effort.\n> \n> When I compared this form to the following, the cognitive load to wrap my\n> head around the code is quite a bit higher there:\n> \n> \t\tif (moff & 0x000000ff) {\n> \t\t\tout[outpos++] = moff >> 0;\n> \t\t\ti |= 0x01;\n> \t\t}\n> \t\tif (moff & 0x0000ff00) {\n> \t\t\tout[outpos++] = moff >> 8;\n> \t\t\ti |= 0x02;\n> \t\t}\n> \t\tif (moff & 0x00ff0000) {\n> \t\t\tout[outpos++] = moff >> 16;\n> \t\t\ti |= 0x04;\n> \t\t}\n> \t\tif (moff & 0xff000000) {\n> \t\t\tout[outpos++] = moff >> 24;\n> \t\t\ti |= 0x08;\n> \t\t}\n> \n> The reason is the visual distance between the near-identical code.\n> \n> Having said that, I do realize that my brain quite possibly works in\n> special ways here and that the general preference is to go with the latter\n> form.\n> \n> Do you have a strong opinion which form to use?\n> \n> Ciao,\n> Johannes\n> \n\n"},{"id":"515093","messageId":"xmqqtt7ghyhr.fsf@gitster.g","threadId":"63188","inReplyTo":"a7506e7b-f86f-426c-b816-540be282518b@gmail.com","subject":"Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-26T10:34:08Z","receivedAt":"2025-03-26T10:34:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> I think that would be clearer, something like the diff below\n>\n> Best Wishes\n>\n> Phillip\n>\n> ---- >8 ----\n>\n> diff --git a/wildmatch.c b/wildmatch.c\n> index 8ea29141bd7..7230544c356 100644\n> --- a/wildmatch.c\n> +++ b/wildmatch.c\n> @@ -223,7 +223,7 @@\n> \t\t\t\t\t\tp_ch = '[';\n> \t\t\t\t\t\tif (t_ch == p_ch)\n> \t\t\t\t\t\t\tmatched = 1;\n> -\t\t\t\t\t\tcontinue;\n> +\t\t\t\t\t\tgoto next;\n> \t\t\t\t\t}\n> \t\t\t\t\tif (CC_EQ(s,i, \"alnum\")) {\n> \t\t\t\t\t\tif (ISALNUM(t_ch))\n> @@ -268,7 +268,10 @@\n> \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n> \t\t\t\t} else if (t_ch == p_ch)\n> \t\t\t\t\tmatched = 1;\n> -\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n> +\t\t\tnext:\n> +\t\t\t\tprev_ch = p_ch;\n> +\t\t\t\tp_ch = *++p;\n> +\t\t\t} while (p_ch != ']');\n> \t\t\tif (matched == negated ||\n> \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n> \t\t\t\treturn WM_NOMATCH;\n>\n\nAh, I missed that \"continue\"; with the \"next:\" label, it looks quite\nclear what is going on.\n\nI like it. Thanks.\n"},{"id":"515103","messageId":"Z+Q5cWAHxxNm8BhK@nand.local","threadId":"63188","inReplyTo":"ea6bfd92-3d09-c611-abdc-dc7bc987446f@gmx.de","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-26T17:29:21Z","receivedAt":"2025-03-26T17:29:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 25, 2025 at 03:12:21PM +0100, Johannes Schindelin wrote:\n> Hi Philip,\n>\n> On Tue, 25 Mar 2025, Philip Oakley wrote:\n>\n> > On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:\n> > > The comma operator\n> > > [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> > > rarely used in C anymore, and typically indicates a typo. Just like in these\n> > > instances, where a semicolon was meant to be used, as there is no need to\n> > > discard the first statement's result here.\n> >\n> > Minor aside: How were these 'discovered'?\n>\n> I am working on a GitHub workflow that uses CodeQL to find such issues,\n> that's how I found them. (I also worked with the CodeQL team to get this\n> query added, way back when I was still working at GitHub.)\n\nNeat. I will be curious to see how the results compare/contrast to what\nthose of us who run Coverity get.\n\nThanks,\nTaylor\n"},{"id":"515104","messageId":"20250326174127.GA2508080@coredump.intra.peff.net","threadId":"63188","inReplyTo":"2f6f31240fe6ce5f8efab662af477540a0f966ca.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-26T17:41:27Z","receivedAt":"2025-03-26T17:41:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> In my setup, clang finds `/usr/local/cuda` and hence the output of\n> `clang -v` ends with this line:\n> \n> \tFound CUDA installation: /usr/local/cuda, version\n> \n> This confuses the `detect-compiler` script because it matches _all_\n> lines that contain the needle \"version\" surrounded by spaces. As a\n> consequence, the `get_family` function returns two lines: \"Ubuntu clang\"\n> and above-mentioned line, which the `case` statement does not handle\n> well and hence reports \"unknown compiler family\" instead of the expected\n> set of \"clang14\", \"clang13\", ..., \"clang1\" output.\n> \n> Let's unconfuse the script by letting it parse the first matching line\n> and ignore the rest.\n\nMakes sense. I wondered if this:\n\n> get_version_line() {\n> -\tLANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n> +\tLANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'\n\nmight be more readable with \"grep -m1\", but it looks like \"-m\" is not in\nPOSIX. So what you wrote is probably safer.\n\n-Peff\n"},{"id":"515105","messageId":"20250326175059.GB2508080@coredump.intra.peff.net","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 00/10] Avoid the comma operator","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-26T17:50:59Z","receivedAt":"2025-03-26T17:51:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:\n\n> The comma operator\n> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\n> rarely used in C anymore, and typically indicates a typo. Just like in these\n> instances, where a semicolon was meant to be used, as there is no need to\n> discard the first statement's result here.\n> \n> Changes since v1:\n> \n> * Use -Wcomma when compiling with clang and with DEVELOPER=1.\n> * Address the remaining instances pointed out by clang (and by Phillip).\n\nThanks for fixing these. I checked the diff against the quick-and-dirty\npatch I posted earlier in the thread, and your resolutions for the\n\"easy\" cases look good (though like others, I'd prefer switching to\nsemicolons for the one in diff-delta.c).\n\nFor the harder cases inside while() conditionals, the rewrites all look\ncorrect to me. I do think that getting rid of the commas often makes the\nresult easier to read, but the discussion on wildmatch shows that it's\neasy to get the transformation wrong. So I'd be happy enough slapping a\n\"(void)\" on that one and moving on with our lives. The goal is not to\nprettify that code (which was not even written for Git in the first\nplace) but to silence -Wcomma false positives so that we can find the\nactual problems.\n\n-Peff\n"},{"id":"515107","messageId":"CAPig+cR+ESNg4tV1G6jbKKeRKABD053qZcG0BoFuQ7aC+1tGYw@mail.gmail.com","threadId":"63188","inReplyTo":"20250326174127.GA2508080@coredump.intra.peff.net","subject":"Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-26T18:07:10Z","receivedAt":"2025-03-26T18:07:22Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Mar 26, 2025 at 1:44 PM Jeff King wrote:\n> On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > Let's unconfuse the script by letting it parse the first matching line\n> > and ignore the rest.\n>\n> Makes sense. I wondered if this:\n>\n> > get_version_line() {\n> > - LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n> > + LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'\n>\n> might be more readable with \"grep -m1\", but it looks like \"-m\" is not in\n> POSIX. So what you wrote is probably safer.\n\nIt's probably an indication that I've done too much `sed` programming,\nbut I find Dscho's version more obvious. That aside, your response\nmade me take a closer look at what Dscho wrote and I noticed that it\nis syntactically flawed, at least for BSD-lineage `sed`. Testing on\nmacOS reveals that this is indeed so:\n\n % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'\n sed: 1: \"/ version /{p;q}\": extra characters at the end of q command\n\nThe problem is that the `q` function takes no arguments, but\nBSD-lineage `sed` thinks that the closing `}` is an argument rather\nthan a terminator. Fixing this requires inserting a terminator after\n`q`, which will be either a newline character or a semicolon. So, the\ncorrect form is:\n\n sed -n '/ version /{p;q;}\n"},{"id":"515110","messageId":"Z+Rg8iz1cV/pSok+@nand.local","threadId":"63188","inReplyTo":"22fe76ff-7940-86bd-af50-c161e3d04864@gmx.de","subject":"Re: [PATCH 0/2] Avoid the comma operator","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-26T20:17:54Z","receivedAt":"2025-03-26T20:17:56Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 25, 2025 at 03:13:47PM +0100, Johannes Schindelin wrote:\n> > It would be great if there was a compiler warning we could enable for\n> > cases where the operator likely isn't intentional. But I couldn't find\n> > any, unfortunately.\n>\n> I was not actually planning on adding the CodeQL workflow to git/git,\n> seeing as its CI is already taking way too much CPU time for my liking.\n> But in `microsoft/git`, I am kind of required to, so we'll catch those\n> issues there.\n\nHeh. I should have read this email from you before replying to the last\none ;-).\n\nI would actually be interested in having CodeQL support \"ship\" for\ncopies of git.git that are hosted on GitHub. I wonder if we could do a\nsimilar trick as in a56b6230d0 (ci: add a GitHub workflow to submit\nCoverity scans, 2023-09-25), where the ENABLE_COVERITY_SCAN_FOR_BRANCHES\nvariable controls whether or not the job actually executes.\n\nThanks,\nTaylor\n"},{"id":"515120","messageId":"Z+RkmLzciwT6ubIE@nand.local","threadId":"63188","inReplyTo":"ad57203b-8925-4f88-add6-763c2d80afac@gmail.com","subject":"Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-26T20:33:28Z","receivedAt":"2025-03-26T20:33:30Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 26, 2025 at 10:17:50AM +0000, Phillip Wood wrote:\n> Hi Johannes\n>\n> On 26/03/2025 07:20, Johannes Schindelin wrote:\n> > Hi Patrick,\n> > On Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n> > > Hm. I think the end result is even more confusing than before. Why don't\n> > > we introduce curly braces here, same as in preceding commits?\n> >\n> > The interleaved -/+ lines make it admittedly hard to see what I meant.\n> > I'll unwind it a bit (presenting only the `moff` part, the same\n> > consideration applies to `msize`):\n> >\n> > \t\tif (moff & 0x000000ff)\n> > \t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n> > \t\tif (moff & 0x0000ff00)\n> > \t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n> > \t\tif (moff & 0x00ff0000)\n> > \t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n> > \t\tif (moff & 0xff000000)\n> > \t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n> >\n> > In this form, it is very obvious (from comparing the right-side half of\n> > the lines) that a shifted version of `moff` is appended to `out` and `i`\n> > gets a bit set, and the correlation between shift width and the set bit\n> > is relatively easy to see and validate (at least my brain has an easy time\n> > here, thanks to the alignment and thanks to visual similarity between the\n> > non-blank parts).\n> >\n> > It is admittedly quite a bit harder not to be distracted by the repetitive\n> > `(void)(out[...` parts to understand and validate the `if` conditions on\n> > the left-hand side,\n>\n> That makes it pretty unreadable for me. If you're worried about the vertical\n> space we could perhaps keep both statements on a single line so that we're\n> only adding a single newline for the closing brace rather than two.\n\nMy $.02 here would be that the form:\n\n if (moff & 0x000000ff) {\n out[outpos++] = moff >> 0;\n i |= 0x01;\n }\n\nis the most readable, and fits within the conventions of our\nCodingGuidelines. I don't really love the idea of writing:\n\n if (moff & 0x000000ff) {\n out[outpos++] = moff >> 0); i |= 0x01;\n }\n\n, since it looks like a single-line if-statement and is thus tempting to\ndrop the braces, which would make the code incorrect, as 'i |= 0x01'\nwould be executed unconditionally. I suppose you could write\n\n if (moff & 0x000000ff) {\n out[outpos++] = moff >> 0); i |= 0x01; }\n\n, but that feels even uglier to me ;-).\n\nThanks,\nTaylor\n"},{"id":"515121","messageId":"Z+RlEwwTvFkUWwkT@nand.local","threadId":"63188","inReplyTo":"dc626f36df34df4897289e508dbf608512a93870.1742945534.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-26T20:35:31Z","receivedAt":"2025-03-26T20:35:34Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c\n> index ec5cc5d2dd1..7672583bf7e 100644\n> --- a/compat/regex/regex_internal.c\n> +++ b/compat/regex/regex_internal.c\n> @@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)\n> for (sbase = dest->nelem + 2 * src->nelem,\n> is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )\n> {\n> - if (dest->elems[id] == src->elems[is])\n> -\tis--, id--;\n> - else if (dest->elems[id] < src->elems[is])\n> + if (dest->elems[id] == src->elems[is]) {\n> +\tis--;\n> +\tid--;\n> + } else if (dest->elems[id] < src->elems[is])\n\nShould the other arms of this conditional have matching curly-braces?\n\nThanks,\nTaylor\n"},{"id":"515127","messageId":"CAPx1GvfyzdF9HasG3aPQA=Jdb03=3YJvNEDXabGK793CCow=HA@mail.gmail.com","threadId":"63188","inReplyTo":"Z+RkmLzciwT6ubIE@nand.local","subject":"Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2025-03-27T01:31:27Z","receivedAt":"2025-03-27T01:31:40Z","isPatch":true,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"(Just picking the latest message to reply to, not really quite right)\n\n> > > if (moff & 0x000000ff)\n> > > (void)(out[outpos++] = moff >> 0), i |= 0x01;\n> > > if (moff & 0x0000ff00)\n> > > (void)(out[outpos++] = moff >> 8), i |= 0x02;\n> > > if (moff & 0x00ff0000)\n> > > (void)(out[outpos++] = moff >> 16), i |= 0x04;\n> > > if (moff & 0xff000000)\n> > > (void)(out[outpos++] = moff >> 24), i |= 0x08;\n\nMight be overkill but:\n\n #define XXX(index) do { \\\n if (moff & (0xffUL << ((index) * 8))) {\n out[outpos++] = moff >> ((index) * 8);\n i |= 1 << (index);\n }\n } while (0)\n\n XXX(0);\n XXX(1);\n XXX(2);\n XXX(3);\n\n #undef XXX\n\nwould do the trick. Pick a proper name for XXX of course.\n\nChris\n"},{"id":"515129","messageId":"20250327051419.GA3042475@coredump.intra.peff.net","threadId":"63188","inReplyTo":"CAPig+cR+ESNg4tV1G6jbKKeRKABD053qZcG0BoFuQ7aC+1tGYw@mail.gmail.com","subject":"Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-27T05:14:19Z","receivedAt":"2025-03-27T05:14:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Mar 26, 2025 at 02:07:10PM -0400, Eric Sunshine wrote:\n\n> On Wed, Mar 26, 2025 at 1:44 PM Jeff King wrote:\n> > On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > > Let's unconfuse the script by letting it parse the first matching line\n> > > and ignore the rest.\n> >\n> > Makes sense. I wondered if this:\n> >\n> > > get_version_line() {\n> > > - LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n> > > + LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'\n> >\n> > might be more readable with \"grep -m1\", but it looks like \"-m\" is not in\n> > POSIX. So what you wrote is probably safer.\n> \n> It's probably an indication that I've done too much `sed` programming,\n> but I find Dscho's version more obvious. That aside, your response\n> made me take a closer look at what Dscho wrote and I noticed that it\n> is syntactically flawed, at least for BSD-lineage `sed`. Testing on\n> macOS reveals that this is indeed so:\n> \n> % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'\n> sed: 1: \"/ version /{p;q}\": extra characters at the end of q command\n> \n> The problem is that the `q` function takes no arguments, but\n> BSD-lineage `sed` thinks that the closing `}` is an argument rather\n> than a terminator. Fixing this requires inserting a terminator after\n> `q`, which will be either a newline character or a semicolon. So, the\n> correct form is:\n> \n> sed -n '/ version /{p;q;}\n\nHeh, I think it was the braces and semicolons that made my spider-sense\ntingle, probably because I've been bitten by those subtleties in the\npast.\n\nI think just \"/foo/p;q\" works on GNU sed, but no idea if it does\nelsewhere. What you wrote seems the safest.\n\n-Peff\n"},{"id":"515130","messageId":"CAPig+cTmcPY9XDJcYc8s+O+a78x5vwBXV76tw7+S2wbOaMAMyA@mail.gmail.com","threadId":"63188","inReplyTo":"20250327051419.GA3042475@coredump.intra.peff.net","subject":"Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2025-03-27T05:21:03Z","receivedAt":"2025-03-27T05:21:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 27, 2025 at 1:14 AM Jeff King wrote:\n> On Wed, Mar 26, 2025 at 02:07:10PM -0400, Eric Sunshine wrote:\n> > It's probably an indication that I've done too much `sed` programming,\n> > but I find Dscho's version more obvious. That aside, your response\n> > made me take a closer look at what Dscho wrote and I noticed that it\n> > is syntactically flawed, at least for BSD-lineage `sed`. Testing on\n> > macOS reveals that this is indeed so:\n> >\n> > % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'\n> > sed: 1: \"/ version /{p;q}\": extra characters at the end of q command\n> >\n> > The problem is that the `q` function takes no arguments, but\n> > BSD-lineage `sed` thinks that the closing `}` is an argument rather\n> > than a terminator. Fixing this requires inserting a terminator after\n> > `q`, which will be either a newline character or a semicolon. So, the\n> > correct form is:\n> >\n> > sed -n '/ version /{p;q;}\n>\n> Heh, I think it was the braces and semicolons that made my spider-sense\n> tingle, probably because I've been bitten by those subtleties in the\n> past.\n>\n> I think just \"/foo/p;q\" works on GNU sed, but no idea if it does\n> elsewhere. What you wrote seems the safest.\n\nThat's not quite the same, though. The patternless `q` will cause\n`sed` to terminate upon reading the first line of input, not upon the\nfirst line which contains \" version \". This matters, for instance, if\nthe first line output by `$CC -v` is not the version string (i.e. it\nmight be a copyright notice).\n"},{"id":"515132","messageId":"20250327063510.GC3042475@coredump.intra.peff.net","threadId":"63188","inReplyTo":"CAPig+cTmcPY9XDJcYc8s+O+a78x5vwBXV76tw7+S2wbOaMAMyA@mail.gmail.com","subject":"Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-03-27T06:35:10Z","receivedAt":"2025-03-27T06:35:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 27, 2025 at 01:21:03AM -0400, Eric Sunshine wrote:\n\n> > I think just \"/foo/p;q\" works on GNU sed, but no idea if it does\n> > elsewhere. What you wrote seems the safest.\n> \n> That's not quite the same, though. The patternless `q` will cause\n> `sed` to terminate upon reading the first line of input, not upon the\n> first line which contains \" version \". This matters, for instance, if\n> the first line output by `$CC -v` is not the version string (i.e. it\n> might be a copyright notice).\n\nOh, duh, you're right. I even tested to make sure were still quitting\nafter matching, but of course that is because we were quitting even when\n_not_ matching.\n\nFor some reason I also thought /foo/pq would work, but it doesn't. I\nwonder if it used to under some implementations, or if I'm simply going\nsenile.\n\n(The point still remains that what you wrote is the correct thing).\n\n-Peff\n"},{"id":"515146","messageId":"04edcf09-5f72-5405-9c13-ef150dada1b8@gmx.de","threadId":"63188","inReplyTo":"Z-O79qs_iL0lYany@pks.im","subject":"Re: [PATCH v2 09/10] clang: warn when the comma operator is used","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-27T10:18:16Z","receivedAt":"2025-03-27T10:18:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Patrick,\n\nOn Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n\n> On Wed, Mar 26, 2025 at 08:50:24AM +0100, Johannes Schindelin wrote:\n> > On Wed, 26 Mar 2025, Patrick Steinhardt wrote:\n> > > diff --git a/meson.build b/meson.build\n> > > index dd231b669b6..a7658d62ea0 100644\n> > > --- a/meson.build\n> > > +++ b/meson.build\n> > > @@ -717,6 +717,7 @@ libgit_dependencies = [ ]\n> > > # Makefile.\n> > > if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'\n> >\n> > This `get_argument_syntax() == 'gcc'` condition catches `clang`? What\n> > other compilers that aren't GCC does it catch?\n>\n> Yes, it does catch Clang as well as the Intel compiler. The list of\n> compilers and their respective syntax can be found at [1]\n>\n> [1]: https://mesonbuild.com/Reference-tables.html#compiler-ids\n\nThank you for this valuable reference!\n\nCiao,\nJohannes\n"},{"id":"515148","messageId":"1c8a3d80-5817-1e2a-de8c-06046d372fea@gmx.de","threadId":"63188","inReplyTo":"Z+RlEwwTvFkUWwkT@nand.local","subject":"Re: [PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2025-03-27T10:29:16Z","receivedAt":"2025-03-27T10:29:22Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Taylor,\n\nOn Wed, 26 Mar 2025, Taylor Blau wrote:\n\n> On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c\n> > index ec5cc5d2dd1..7672583bf7e 100644\n> > --- a/compat/regex/regex_internal.c\n> > +++ b/compat/regex/regex_internal.c\n> > @@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)\n> > for (sbase = dest->nelem + 2 * src->nelem,\n> > is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )\n> > {\n> > - if (dest->elems[id] == src->elems[is])\n> > -\tis--, id--;\n> > - else if (dest->elems[id] < src->elems[is])\n> > + if (dest->elems[id] == src->elems[is]) {\n> > +\tis--;\n> > +\tid--;\n> > + } else if (dest->elems[id] < src->elems[is])\n>\n> Should the other arms of this conditional have matching curly-braces?\n\nNo. Have a look around in that file, that's not the coding convention.\n\nHowever, a valid concern is that I used Git's convention for the curly\nbrackets (appending the open bracket to the `if` line), which is _not_ the\nconvention used in this file, which uses GNU conventions instead. I will\naddress that concern in the next iteration.\n\nCiao,\nJohannes\n"},{"id":"515184","messageId":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v2.git.1742945534.gitgitgadget@gmail.com","subject":"[PATCH v3 00/10] Avoid the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:52:53Z","receivedAt":"2025-03-27T11:53:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The comma operator\n[https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is\nrarely used in C anymore, and typically indicates a typo. Just like in these\ninstances, where a semicolon was meant to be used, as there is no need to\ndiscard the first statement's result here.\n\nChanges since v2:\n\n * Made the sed construct in detect-compiler portable (thanks, Eric\n Sunshine!)\n * The majority of the feedback disagreed with the more compact format in\n diff-delta.c, so I changed it to the long format (thanks, Phillip Wood!)\n * The more succinct and safer, but less readable, cast in the loop\n condition of the dowild() function was replaced with the goto-based\n alternative I had mentioned as a possibility in the commit message\n (thanks, Phillip Wood!)\n * I adjusted the style of my compat/regex/ patch to the surrounding code's.\n * The -Wcomma option is now used in Meson-based clang builds, too (thanks,\n Patrick Steinhardt!)\n\nChanges since v1:\n\n * Use -Wcomma when compiling with clang and with DEVELOPER=1.\n * Address the remaining instances pointed out by clang (and by Phillip).\n\nJohannes Schindelin (10):\n remote-curl: avoid using the comma operator unnecessarily\n rebase: avoid using the comma operator unnecessarily\n kwset: avoid using the comma operator unnecessarily\n clar: avoid using the comma operator unnecessarily\n xdiff: avoid using the comma operator unnecessarily\n diff-delta: avoid using the comma operator\n wildmatch: avoid using of the comma operator\n compat/regex: explicitly mark intentional use of the comma operator\n clang: warn when the comma operator is used\n detect-compiler: detect clang even if it found CUDA\n\n builtin/rebase.c | 2 +-\n compat/regex/regex_internal.c | 5 +++-\n compat/regex/regexec.c | 2 +-\n config.mak.dev | 4 +++\n detect-compiler | 2 +-\n diff-delta.c | 38 +++++++++++++++---------\n kwset.c | 54 +++++++++++++++++++----------------\n meson.build | 1 +\n remote-curl.c | 4 +--\n t/unit-tests/clar/clar/fs.h | 10 +++++--\n wildmatch.c | 7 +++--\n xdiff/xdiffi.c | 12 +++++---\n 12 files changed, 89 insertions(+), 52 deletions(-)\n\n\nbase-commit: 683c54c999c301c2cd6f715c411407c413b1d84e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1889\n\nRange-diff vs v2:\n\n 1: 913c7a0d296 = 1: 913c7a0d296 remote-curl: avoid using the comma operator unnecessarily\n 2: 37ff88b8275 = 2: 37ff88b8275 rebase: avoid using the comma operator unnecessarily\n 3: f601f4e74a5 = 3: f601f4e74a5 kwset: avoid using the comma operator unnecessarily\n 4: f60ebe376e1 = 4: f60ebe376e1 clar: avoid using the comma operator unnecessarily\n 5: 7239078413f = 5: 7239078413f xdiff: avoid using the comma operator unnecessarily\n 6: 5e0e8325620 ! 6: 045d695d73e diff-delta: explicitly mark intentional use of the comma operator\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - diff-delta: explicitly mark intentional use of the comma operator\n + diff-delta: avoid using the comma operator\n \n The comma operator is a somewhat obscure C feature that is often used by\n mistake and can even cause unintentional code flow. That is why the\n @@ Commit message\n Intentional uses include situations where one wants to avoid curly\n brackets around multiple statements that need to be guarded by a\n condition. This is the case here, as the repetitive nature of the\n - statements is easier to see for a human reader this way.\n + statements is easier to see for a human reader this way. At least in my\n + opinion.\n \n - To mark this usage as intentional, the return value of the statement\n - before the comma needs to be cast to `void`, which we do here.\n + However, opinions on this differ wildly, take 10 people and you have 10\n + different preferences.\n \n + On the Git mailing list, it seems that the consensus is to use the long\n + form instead, so let's do just that.\n +\n + Suggested-by: Phillip Wood \n Signed-off-by: Johannes Schindelin \n \n ## diff-delta.c ##\n @@ diff-delta.c: create_delta(const struct delta_index *index,\n + \t\t\top = out + outpos++;\n \t\t\ti = 0x80;\n \n - \t\t\tif (moff & 0x000000ff)\n +-\t\t\tif (moff & 0x000000ff)\n -\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n -+\t\t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n - \t\t\tif (moff & 0x0000ff00)\n +-\t\t\tif (moff & 0x0000ff00)\n -\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n -+\t\t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n - \t\t\tif (moff & 0x00ff0000)\n +-\t\t\tif (moff & 0x00ff0000)\n -\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n -+\t\t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n - \t\t\tif (moff & 0xff000000)\n +-\t\t\tif (moff & 0xff000000)\n -\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n -+\t\t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n - \n - \t\t\tif (msize & 0x00ff)\n +-\n +-\t\t\tif (msize & 0x00ff)\n -\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n -+\t\t\t\t(void)(out[outpos++] = msize >> 0), i |= 0x10;\n - \t\t\tif (msize & 0xff00)\n +-\t\t\tif (msize & 0xff00)\n -\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n -+\t\t\t\t(void)(out[outpos++] = msize >> 8), i |= 0x20;\n ++\t\t\tif (moff & 0x000000ff) {\n ++\t\t\t\tout[outpos++] = moff >> 0;\n ++\t\t\t\ti |= 0x01;\n ++\t\t\t}\n ++\t\t\tif (moff & 0x0000ff00) {\n ++\t\t\t\tout[outpos++] = moff >> 8;\n ++\t\t\t\ti |= 0x02;\n ++\t\t\t}\n ++\t\t\tif (moff & 0x00ff0000) {\n ++\t\t\t\tout[outpos++] = moff >> 16;\n ++\t\t\t\ti |= 0x04;\n ++\t\t\t}\n ++\t\t\tif (moff & 0xff000000) {\n ++\t\t\t\tout[outpos++] = moff >> 24;\n ++\t\t\t\ti |= 0x08;\n ++\t\t\t}\n ++\n ++\t\t\tif (msize & 0x00ff) {\n ++\t\t\t\tout[outpos++] = msize >> 0;\n ++\t\t\t\ti |= 0x10;\n ++\t\t\t}\n ++\t\t\tif (msize & 0xff00) {\n ++\t\t\t\tout[outpos++] = msize >> 8;\n ++\t\t\t\ti |= 0x20;\n ++\t\t\t}\n \n \t\t\t*op = i;\n \n 7: 9a6de12b807 ! 7: 1d0ce59cb68 wildmatch: explicitly mark intentional use of the comma operator\n @@ Metadata\n Author: Johannes Schindelin \n \n ## Commit message ##\n - wildmatch: explicitly mark intentional use of the comma operator\n + wildmatch: avoid using of the comma operator\n \n The comma operator is a somewhat obscure C feature that is often used by\n mistake and can even cause unintentional code flow. That is why the\n `-Wcomma` option of clang was introduced: To identify unintentional uses\n of the comma operator.\n \n - To mark such a usage as intentional, the value needs to be cast to\n - `void`, which we do here.\n -\n In this instance, the usage is intentional because it allows storing the\n value of the current character as `prev_ch` before making the next\n character the current one, all of which happens in the loop condition\n that lets the loop stop at a closing bracket.\n \n - The alternative to using the comma operator would be to move those\n + However, it is hard to read.\n +\n + The chosen alternative to using the comma operator is to move those\n assignments from the condition into the loop body; In this particular\n - case that would require the assignments to either be duplicated or to\n - introduce and use a `goto` target before the assignments, though,\n - because the loop body contains a `continue` for the case where a\n - character class is found that starts with `[:` but does not end in `:]`\n - (and the assignments should occur even when that code path is taken).\n + case that requires special care because the loop body contains a\n + `continue` for the case where a character class is found that starts\n + with `[:` but does not end in `:]` (and the assignments should occur\n + even when that code path is taken), which needs to be turned into a\n + `goto`.\n \n + Helped-by: Phillip Wood \n Signed-off-by: Johannes Schindelin \n \n ## wildmatch.c ##\n +@@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n + \t\t\t\t\t\tp_ch = '[';\n + \t\t\t\t\t\tif (t_ch == p_ch)\n + \t\t\t\t\t\t\tmatched = 1;\n +-\t\t\t\t\t\tcontinue;\n ++\t\t\t\t\t\tgoto next;\n + \t\t\t\t\t}\n + \t\t\t\t\tif (CC_EQ(s,i, \"alnum\")) {\n + \t\t\t\t\t\tif (ISALNUM(t_ch))\n @@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n \t\t\t\t} else if (t_ch == p_ch)\n \t\t\t\t\tmatched = 1;\n -\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n -+\t\t\t} while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');\n ++next:\n ++\t\t\t\tprev_ch = p_ch;\n ++\t\t\t\tp_ch = *++p;\n ++\t\t\t} while (p_ch != ']');\n \t\t\tif (matched == negated ||\n \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n \t\t\t\treturn WM_NOMATCH;\n 8: dc626f36df3 ! 8: b8405f3d237 compat/regex: explicitly mark intentional use of the comma operator\n @@ Commit message\n \n ## compat/regex/regex_internal.c ##\n @@ compat/regex/regex_internal.c: re_node_set_merge (re_node_set *dest, const re_node_set *src)\n - for (sbase = dest->nelem + 2 * src->nelem,\n is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )\n {\n -- if (dest->elems[id] == src->elems[is])\n + if (dest->elems[id] == src->elems[is])\n -\tis--, id--;\n -- else if (dest->elems[id] < src->elems[is])\n -+ if (dest->elems[id] == src->elems[is]) {\n -+\tis--;\n -+\tid--;\n -+ } else if (dest->elems[id] < src->elems[is])\n ++\t{\n ++\t is--;\n ++\t id--;\n ++\t}\n + else if (dest->elems[id] < src->elems[is])\n \tdest->elems[--sbase] = src->elems[is--];\n else /* if (dest->elems[id] > src->elems[is]) */\n - \t--id;\n \n ## compat/regex/regexec.c ##\n @@ compat/regex/regexec.c: sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,\n 9: 91f86c3aba9 ! 9: 6b6cd556465 clang: warn when the comma operator is used\n @@ Commit message\n warn about code using the comma operator (because it is typically\n unintentional and wants to use the semicolon instead).\n \n + Helped-by: Patrick Steinhardt \n Signed-off-by: Johannes Schindelin \n \n ## config.mak.dev ##\n @@ config.mak.dev: DEVELOPER_CFLAGS += -Wvla\n ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n endif\n +\n + ## meson.build ##\n +@@ meson.build: libgit_dependencies = [ ]\n + # Makefile.\n + if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'\n + foreach cflag : [\n ++ '-Wcomma',\n + '-Wdeclaration-after-statement',\n + '-Wformat-security',\n + '-Wold-style-definition',\n 10: 2f6f31240fe ! 10: 77f1dcaca1c detect-compiler: detect clang even if it found CUDA\n @@ Commit message\n Let's unconfuse the script by letting it parse the first matching line\n and ignore the rest.\n \n + Helped-by: Eric Sunshine \n Signed-off-by: Johannes Schindelin \n \n ## detect-compiler ##\n @@ detect-compiler: CC=\"$*\"\n # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n get_version_line() {\n -\tLANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n -+\tLANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'\n ++\tLANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}'\n }\n \n get_family() {\n\n-- \ngitgitgadget\n"},{"id":"515185","messageId":"37ff88b8275cf4d6b0c715a99f4572e70d6e3729.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 02/10] rebase: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:52:55Z","receivedAt":"2025-03-27T11:53:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n builtin/rebase.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d4715ed35d7..62bdf7276f7 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,\n \tstrbuf_addf(&msg, \"%s (start): checkout %s\",\n \t\t options.reflog_action, options.onto_name);\n \tropts.oid = &options.onto->object.oid;\n-\tropts.orig_head = &options.orig_head->object.oid,\n+\tropts.orig_head = &options.orig_head->object.oid;\n \tropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |\n \t\t\tRESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n \tropts.head_msg = msg.buf;\n-- \ngitgitgadget\n\n"},{"id":"515186","messageId":"913c7a0d29699c2bd80fefcdc00879c400f1e7db.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 01/10] remote-curl: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:52:54Z","receivedAt":"2025-03-27T11:53:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n remote-curl.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 1273507a96c..590b228f67f 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads,\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-upload-pack\",\n+\trpc.service_name = \"git-upload-pack\";\n \trpc.gzip_request = 1;\n \n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n@@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)\n \tpacket_buf_flush(&preamble);\n \n \tmemset(&rpc, 0, sizeof(rpc));\n-\trpc.service_name = \"git-receive-pack\",\n+\trpc.service_name = \"git-receive-pack\";\n \n \terr = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);\n \tif (rpc_result.len)\n-- \ngitgitgadget\n\n"},{"id":"515187","messageId":"f601f4e74a5e8146f2926a725f3d1522b928eb51.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 03/10] kwset: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:52:56Z","receivedAt":"2025-03-27T11:53:10Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n kwset.c | 54 +++++++++++++++++++++++++++++-------------------------\n 1 file changed, 29 insertions(+), 25 deletions(-)\n\ndiff --git a/kwset.c b/kwset.c\nindex 1714eada608..064329434e5 100644\n--- a/kwset.c\n+++ b/kwset.c\n@@ -197,10 +197,13 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n while (link && label != link->label)\n \t{\n \t links[depth] = link;\n-\t if (label < link->label)\n-\t dirs[depth++] = L, link = link->llink;\n-\t else\n-\t dirs[depth++] = R, link = link->rlink;\n+\t if (label < link->label) {\n+\t dirs[depth++] = L;\n+\t link = link->llink;\n+\t } else {\n+\t dirs[depth++] = R;\n+\t link = link->rlink;\n+\t }\n \t}\n \n /* The current character doesn't have an outgoing link at\n@@ -257,14 +260,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n \t\t switch (dirs[depth + 1])\n \t\t {\n \t\t case L:\n-\t\t r = links[depth], t = r->llink, rl = t->rlink;\n-\t\t t->rlink = r, r->llink = rl;\n+\t\t r = links[depth]; t = r->llink; rl = t->rlink;\n+\t\t t->rlink = r; r->llink = rl;\n \t\t t->balance = r->balance = 0;\n \t\t break;\n \t\t case R:\n-\t\t r = links[depth], l = r->llink, t = l->rlink;\n-\t\t rl = t->rlink, lr = t->llink;\n-\t\t t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;\n+\t\t r = links[depth]; l = r->llink; t = l->rlink;\n+\t\t rl = t->rlink; lr = t->llink;\n+\t\t t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;\n \t\t l->balance = t->balance != 1 ? 0 : -1;\n \t\t r->balance = t->balance != (char) -1 ? 0 : 1;\n \t\t t->balance = 0;\n@@ -277,14 +280,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)\n \t\t switch (dirs[depth + 1])\n \t\t {\n \t\t case R:\n-\t\t l = links[depth], t = l->rlink, lr = t->llink;\n-\t\t t->llink = l, l->rlink = lr;\n+\t\t l = links[depth]; t = l->rlink; lr = t->llink;\n+\t\t t->llink = l; l->rlink = lr;\n \t\t t->balance = l->balance = 0;\n \t\t break;\n \t\t case L:\n-\t\t l = links[depth], r = l->rlink, t = r->llink;\n-\t\t lr = t->llink, rl = t->rlink;\n-\t\t t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;\n+\t\t l = links[depth]; r = l->rlink; t = r->llink;\n+\t\t lr = t->llink; rl = t->rlink;\n+\t\t t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;\n \t\t l->balance = t->balance != 1 ? 0 : -1;\n \t\t r->balance = t->balance != (char) -1 ? 0 : 1;\n \t\t t->balance = 0;\n@@ -567,22 +570,22 @@ bmexec (kwset_t kws, char const *text, size_t size)\n {\n \twhile (tp <= ep)\n \t {\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t if (d == 0)\n \t goto found;\n-\t d = d1[U(tp[-1])], tp += d;\n-\t d = d1[U(tp[-1])], tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n+\t d = d1[U(tp[-1])]; tp += d;\n \t }\n \tbreak;\n found:\n@@ -649,7 +652,8 @@ cwexec (kwset_t kws, char const *text, size_t len, struct kwsmatch *kwsmatch)\n mch = NULL;\n else\n {\n- mch = text, accept = kwset->trie;\n+ mch = text;\n+ accept = kwset->trie;\n goto match;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"515188","messageId":"f60ebe376e10d7741f6bd657874a17f6c09d4477.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 04/10] clar: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:52:57Z","receivedAt":"2025-03-27T11:53:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. In this instance, it\nmakes the code harder to read than necessary, too. Better use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n t/unit-tests/clar/clar/fs.h | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/t/unit-tests/clar/clar/fs.h b/t/unit-tests/clar/clar/fs.h\nindex 8b206179fc4..2203743fb48 100644\n--- a/t/unit-tests/clar/clar/fs.h\n+++ b/t/unit-tests/clar/clar/fs.h\n@@ -376,9 +376,12 @@ fs_copydir_helper(const char *source, const char *dest, int dest_mode)\n \tmkdir(dest, dest_mode);\n \n \tcl_assert_(source_dir = opendir(source), \"Could not open source dir\");\n-\twhile ((d = (errno = 0, readdir(source_dir))) != NULL) {\n+\tfor (;;) {\n \t\tchar *child;\n \n+\t\terrno = 0;\n+\t\tif ((d = readdir(source_dir)) == NULL)\n+\t\t\tbreak;\n \t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n \t\t\tcontinue;\n \n@@ -479,9 +482,12 @@ fs_rmdir_helper(const char *path)\n \tstruct dirent *d;\n \n \tcl_assert_(dir = opendir(path), \"Could not open dir\");\n-\twhile ((d = (errno = 0, readdir(dir))) != NULL) {\n+\tfor (;;) {\n \t\tchar *child;\n \n+\t\terrno = 0;\n+\t\tif ((d = readdir(dir)) == NULL)\n+\t\t\tbreak;\n \t\tif (!strcmp(d->d_name, \".\") || !strcmp(d->d_name, \"..\"))\n \t\t\tcontinue;\n \n-- \ngitgitgadget\n\n"},{"id":"515189","messageId":"7239078413f6a223105939af1b56e79b9d302f1f.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 05/10] xdiff: avoid using the comma operator unnecessarily","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:52:58Z","receivedAt":"2025-03-27T11:53:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. While the code in\nthis patch used the comma operator intentionally (to avoid curly\nbrackets around two statements, each, that want to be guarded by a\ncondition), it is better to surround it with curly brackets and to use a\nsemicolon instead.\n\nSigned-off-by: Johannes Schindelin \n---\n xdiff/xdiffi.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c\nindex 8889b8b62a1..5a96e36dfbe 100644\n--- a/xdiff/xdiffi.c\n+++ b/xdiff/xdiffi.c\n@@ -211,8 +211,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n \t\t\tfor (d = fmax; d >= fmin; d -= 2) {\n \t\t\t\ti1 = XDL_MIN(kvdf[d], lim1);\n \t\t\t\ti2 = i1 - d;\n-\t\t\t\tif (lim2 < i2)\n-\t\t\t\t\ti1 = lim2 + d, i2 = lim2;\n+\t\t\t\tif (lim2 < i2) {\n+\t\t\t\t\ti1 = lim2 + d;\n+\t\t\t\t\ti2 = lim2;\n+\t\t\t\t}\n \t\t\t\tif (fbest < i1 + i2) {\n \t\t\t\t\tfbest = i1 + i2;\n \t\t\t\t\tfbest1 = i1;\n@@ -223,8 +225,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,\n \t\t\tfor (d = bmax; d >= bmin; d -= 2) {\n \t\t\t\ti1 = XDL_MAX(off1, kvdb[d]);\n \t\t\t\ti2 = i1 - d;\n-\t\t\t\tif (i2 < off2)\n-\t\t\t\t\ti1 = off2 + d, i2 = off2;\n+\t\t\t\tif (i2 < off2) {\n+\t\t\t\t\ti1 = off2 + d;\n+\t\t\t\t\ti2 = off2;\n+\t\t\t\t}\n \t\t\t\tif (i1 + i2 < bbest) {\n \t\t\t\t\tbbest = i1 + i2;\n \t\t\t\t\tbbest1 = i1;\n-- \ngitgitgadget\n\n"},{"id":"515190","messageId":"045d695d73e059ac9d56570c9a2e1176b9c0c1d7.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 06/10] diff-delta: avoid using the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:52:59Z","receivedAt":"2025-03-27T11:53:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. That is why the\n`-Wcomma` option of clang was introduced: To identify unintentional uses\nof the comma operator.\n\nIntentional uses include situations where one wants to avoid curly\nbrackets around multiple statements that need to be guarded by a\ncondition. This is the case here, as the repetitive nature of the\nstatements is easier to see for a human reader this way. At least in my\nopinion.\n\nHowever, opinions on this differ wildly, take 10 people and you have 10\ndifferent preferences.\n\nOn the Git mailing list, it seems that the consensus is to use the long\nform instead, so let's do just that.\n\nSuggested-by: Phillip Wood \nSigned-off-by: Johannes Schindelin \n---\n diff-delta.c | 38 +++++++++++++++++++++++++-------------\n 1 file changed, 25 insertions(+), 13 deletions(-)\n\ndiff --git a/diff-delta.c b/diff-delta.c\nindex a4faf73829b..71d37368d68 100644\n--- a/diff-delta.c\n+++ b/diff-delta.c\n@@ -438,19 +438,31 @@ create_delta(const struct delta_index *index,\n \t\t\top = out + outpos++;\n \t\t\ti = 0x80;\n \n-\t\t\tif (moff & 0x000000ff)\n-\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n-\t\t\tif (moff & 0x0000ff00)\n-\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n-\t\t\tif (moff & 0x00ff0000)\n-\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n-\t\t\tif (moff & 0xff000000)\n-\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n-\n-\t\t\tif (msize & 0x00ff)\n-\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n-\t\t\tif (msize & 0xff00)\n-\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n+\t\t\tif (moff & 0x000000ff) {\n+\t\t\t\tout[outpos++] = moff >> 0;\n+\t\t\t\ti |= 0x01;\n+\t\t\t}\n+\t\t\tif (moff & 0x0000ff00) {\n+\t\t\t\tout[outpos++] = moff >> 8;\n+\t\t\t\ti |= 0x02;\n+\t\t\t}\n+\t\t\tif (moff & 0x00ff0000) {\n+\t\t\t\tout[outpos++] = moff >> 16;\n+\t\t\t\ti |= 0x04;\n+\t\t\t}\n+\t\t\tif (moff & 0xff000000) {\n+\t\t\t\tout[outpos++] = moff >> 24;\n+\t\t\t\ti |= 0x08;\n+\t\t\t}\n+\n+\t\t\tif (msize & 0x00ff) {\n+\t\t\t\tout[outpos++] = msize >> 0;\n+\t\t\t\ti |= 0x10;\n+\t\t\t}\n+\t\t\tif (msize & 0xff00) {\n+\t\t\t\tout[outpos++] = msize >> 8;\n+\t\t\t\ti |= 0x20;\n+\t\t\t}\n \n \t\t\t*op = i;\n \n-- \ngitgitgadget\n\n"},{"id":"515191","messageId":"1d0ce59cb684c2878f1a0db9daeae23ef4abb763.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 07/10] wildmatch: avoid using of the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:53:00Z","receivedAt":"2025-03-27T11:53:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. That is why the\n`-Wcomma` option of clang was introduced: To identify unintentional uses\nof the comma operator.\n\nIn this instance, the usage is intentional because it allows storing the\nvalue of the current character as `prev_ch` before making the next\ncharacter the current one, all of which happens in the loop condition\nthat lets the loop stop at a closing bracket.\n\nHowever, it is hard to read.\n\nThe chosen alternative to using the comma operator is to move those\nassignments from the condition into the loop body; In this particular\ncase that requires special care because the loop body contains a\n`continue` for the case where a character class is found that starts\nwith `[:` but does not end in `:]` (and the assignments should occur\neven when that code path is taken), which needs to be turned into a\n`goto`.\n\nHelped-by: Phillip Wood \nSigned-off-by: Johannes Schindelin \n---\n wildmatch.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/wildmatch.c b/wildmatch.c\nindex 8ea29141bd7..69a2ae7000d 100644\n--- a/wildmatch.c\n+++ b/wildmatch.c\n@@ -223,7 +223,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\t\tp_ch = '[';\n \t\t\t\t\t\tif (t_ch == p_ch)\n \t\t\t\t\t\t\tmatched = 1;\n-\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t\tgoto next;\n \t\t\t\t\t}\n \t\t\t\t\tif (CC_EQ(s,i, \"alnum\")) {\n \t\t\t\t\t\tif (ISALNUM(t_ch))\n@@ -268,7 +268,10 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n \t\t\t\t} else if (t_ch == p_ch)\n \t\t\t\t\tmatched = 1;\n-\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n+next:\n+\t\t\t\tprev_ch = p_ch;\n+\t\t\t\tp_ch = *++p;\n+\t\t\t} while (p_ch != ']');\n \t\t\tif (matched == negated ||\n \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n \t\t\t\treturn WM_NOMATCH;\n-- \ngitgitgadget\n\n"},{"id":"515192","messageId":"b8405f3d23734b860bd222705d498bde63136432.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 08/10] compat/regex: explicitly mark intentional use of the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:53:01Z","receivedAt":"2025-03-27T11:53:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe comma operator is a somewhat obscure C feature that is often used by\nmistake and can even cause unintentional code flow. That is why the\n`-Wcomma` option of clang was introduced: To identify unintentional uses\nof the comma operator.\n\nIn the `compat/regex/` code, the comma operator is used twice, once to\navoid surrounding two conditional statements with curly brackets, the\nother one to increment two counters simultaneously in a `do ... while`\ncondition.\n\nThe first one is replaced with a proper conditional block, surrounded by\ncurly brackets.\n\nThe second one would be harder to replace because the loop contains two\n`continue`s. Therefore, the second one is marked as intentional by\ncasting the value-to-discard to `void`.\n\nSigned-off-by: Johannes Schindelin \n---\n compat/regex/regex_internal.c | 5 ++++-\n compat/regex/regexec.c | 2 +-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c\nindex ec5cc5d2dd1..4a4f849629a 100644\n--- a/compat/regex/regex_internal.c\n+++ b/compat/regex/regex_internal.c\n@@ -1232,7 +1232,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)\n is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )\n {\n if (dest->elems[id] == src->elems[is])\n-\tis--, id--;\n+\t{\n+\t is--;\n+\t id--;\n+\t}\n else if (dest->elems[id] < src->elems[is])\n \tdest->elems[--sbase] = src->elems[is--];\n else /* if (dest->elems[id] > src->elems[is]) */\ndiff --git a/compat/regex/regexec.c b/compat/regex/regexec.c\nindex 2eeec82f407..c08f1bbe1f5 100644\n--- a/compat/regex/regexec.c\n+++ b/compat/regex/regexec.c\n@@ -2210,7 +2210,7 @@ sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,\n \t /* mctx->bkref_ents may have changed, reload the pointer. */\n \t entry = mctx->bkref_ents + enabled_idx;\n \t}\n- while (enabled_idx++, entry++->more);\n+ while ((void)enabled_idx++, entry++->more);\n }\n err = REG_NOERROR;\n free_return:\n-- \ngitgitgadget\n\n"},{"id":"515193","messageId":"6b6cd556465f21e43536706c88c49f8790a2dc5f.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 09/10] clang: warn when the comma operator is used","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:53:02Z","receivedAt":"2025-03-27T11:53:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nWhen compiling Git using `clang`, the `-Wcomma` option can be used to\nwarn about code using the comma operator (because it is typically\nunintentional and wants to use the semicolon instead).\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Johannes Schindelin \n---\n config.mak.dev | 4 ++++\n meson.build | 1 +\n 2 files changed, 5 insertions(+)\n\ndiff --git a/config.mak.dev b/config.mak.dev\nindex 0fd8cc4d355..31423638169 100644\n--- a/config.mak.dev\n+++ b/config.mak.dev\n@@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla\n DEVELOPER_CFLAGS += -Wwrite-strings\n DEVELOPER_CFLAGS += -fno-common\n \n+ifneq ($(filter clang9,$(COMPILER_FEATURES)),)\n+DEVELOPER_CFLAGS += -Wcomma\n+endif\n+\n ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n endif\ndiff --git a/meson.build b/meson.build\nindex efe2871c9db..fd8c05dec91 100644\n--- a/meson.build\n+++ b/meson.build\n@@ -715,6 +715,7 @@ libgit_dependencies = [ ]\n # Makefile.\n if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'\n foreach cflag : [\n+ '-Wcomma',\n '-Wdeclaration-after-statement',\n '-Wformat-security',\n '-Wold-style-definition',\n-- \ngitgitgadget\n\n"},{"id":"515194","messageId":"77f1dcaca1c9df9e24880680311e5bb3eaeec1f8.1743076383.git.gitgitgadget@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"[PATCH v3 10/10] detect-compiler: detect clang even if it found CUDA","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-03-27T11:53:03Z","receivedAt":"2025-03-27T11:53:18Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nIn my setup, clang finds `/usr/local/cuda` and hence the output of\n`clang -v` ends with this line:\n\n\tFound CUDA installation: /usr/local/cuda, version\n\nThis confuses the `detect-compiler` script because it matches _all_\nlines that contain the needle \"version\" surrounded by spaces. As a\nconsequence, the `get_family` function returns two lines: \"Ubuntu clang\"\nand above-mentioned line, which the `case` statement does not handle\nwell and hence reports \"unknown compiler family\" instead of the expected\nset of \"clang14\", \"clang13\", ..., \"clang1\" output.\n\nLet's unconfuse the script by letting it parse the first matching line\nand ignore the rest.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Johannes Schindelin \n---\n detect-compiler | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/detect-compiler b/detect-compiler\nindex a87650b71bb..124ebdd4c9d 100755\n--- a/detect-compiler\n+++ b/detect-compiler\n@@ -9,7 +9,7 @@ CC=\"$*\"\n #\n # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n get_version_line() {\n-\tLANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n+\tLANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}'\n }\n \n get_family() {\n-- \ngitgitgadget\n"},{"id":"515205","messageId":"504e63df-77d2-4cd0-bdcc-bd9949d34ce5@gmail.com","threadId":"63188","inReplyTo":"pull.1889.v3.git.1743076383.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 00/10] Avoid the comma operator","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-27T15:07:17Z","receivedAt":"2025-03-27T15:07:23Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Johannes\n\nOn 27/03/2025 11:52, Johannes Schindelin via GitGitGadget wrote:\n> \n> Changes since v2:\n> \n> * Made the sed construct in detect-compiler portable (thanks, Eric\n> Sunshine!)\n> * The majority of the feedback disagreed with the more compact format in\n> diff-delta.c, so I changed it to the long format (thanks, Phillip Wood!)\n> * The more succinct and safer, but less readable, cast in the loop\n> condition of the dowild() function was replaced with the goto-based\n> alternative I had mentioned as a possibility in the commit message\n> (thanks, Phillip Wood!)\n> * I adjusted the style of my compat/regex/ patch to the surrounding code's.\n> * The -Wcomma option is now used in Meson-based clang builds, too (thanks,\n> Patrick Steinhardt!)\n\nThe range-diff below looks good to me, thanks for making our code base \nclearer.\n\nBest Wishes\n\nPhillip\n\n> Range-diff vs v2:\n> \n> 1: 913c7a0d296 = 1: 913c7a0d296 remote-curl: avoid using the comma operator unnecessarily\n> 2: 37ff88b8275 = 2: 37ff88b8275 rebase: avoid using the comma operator unnecessarily\n> 3: f601f4e74a5 = 3: f601f4e74a5 kwset: avoid using the comma operator unnecessarily\n> 4: f60ebe376e1 = 4: f60ebe376e1 clar: avoid using the comma operator unnecessarily\n> 5: 7239078413f = 5: 7239078413f xdiff: avoid using the comma operator unnecessarily\n> 6: 5e0e8325620 ! 6: 045d695d73e diff-delta: explicitly mark intentional use of the comma operator\n> @@ Metadata\n> Author: Johannes Schindelin \n> \n> ## Commit message ##\n> - diff-delta: explicitly mark intentional use of the comma operator\n> + diff-delta: avoid using the comma operator\n> \n> The comma operator is a somewhat obscure C feature that is often used by\n> mistake and can even cause unintentional code flow. That is why the\n> @@ Commit message\n> Intentional uses include situations where one wants to avoid curly\n> brackets around multiple statements that need to be guarded by a\n> condition. This is the case here, as the repetitive nature of the\n> - statements is easier to see for a human reader this way.\n> + statements is easier to see for a human reader this way. At least in my\n> + opinion.\n> \n> - To mark this usage as intentional, the return value of the statement\n> - before the comma needs to be cast to `void`, which we do here.\n> + However, opinions on this differ wildly, take 10 people and you have 10\n> + different preferences.\n> \n> + On the Git mailing list, it seems that the consensus is to use the long\n> + form instead, so let's do just that.\n> +\n> + Suggested-by: Phillip Wood \n> Signed-off-by: Johannes Schindelin \n> \n> ## diff-delta.c ##\n> @@ diff-delta.c: create_delta(const struct delta_index *index,\n> + \t\t\top = out + outpos++;\n> \t\t\ti = 0x80;\n> \n> - \t\t\tif (moff & 0x000000ff)\n> +-\t\t\tif (moff & 0x000000ff)\n> -\t\t\t\tout[outpos++] = moff >> 0, i |= 0x01;\n> -+\t\t\t\t(void)(out[outpos++] = moff >> 0), i |= 0x01;\n> - \t\t\tif (moff & 0x0000ff00)\n> +-\t\t\tif (moff & 0x0000ff00)\n> -\t\t\t\tout[outpos++] = moff >> 8, i |= 0x02;\n> -+\t\t\t\t(void)(out[outpos++] = moff >> 8), i |= 0x02;\n> - \t\t\tif (moff & 0x00ff0000)\n> +-\t\t\tif (moff & 0x00ff0000)\n> -\t\t\t\tout[outpos++] = moff >> 16, i |= 0x04;\n> -+\t\t\t\t(void)(out[outpos++] = moff >> 16), i |= 0x04;\n> - \t\t\tif (moff & 0xff000000)\n> +-\t\t\tif (moff & 0xff000000)\n> -\t\t\t\tout[outpos++] = moff >> 24, i |= 0x08;\n> -+\t\t\t\t(void)(out[outpos++] = moff >> 24), i |= 0x08;\n> -\n> - \t\t\tif (msize & 0x00ff)\n> +-\n> +-\t\t\tif (msize & 0x00ff)\n> -\t\t\t\tout[outpos++] = msize >> 0, i |= 0x10;\n> -+\t\t\t\t(void)(out[outpos++] = msize >> 0), i |= 0x10;\n> - \t\t\tif (msize & 0xff00)\n> +-\t\t\tif (msize & 0xff00)\n> -\t\t\t\tout[outpos++] = msize >> 8, i |= 0x20;\n> -+\t\t\t\t(void)(out[outpos++] = msize >> 8), i |= 0x20;\n> ++\t\t\tif (moff & 0x000000ff) {\n> ++\t\t\t\tout[outpos++] = moff >> 0;\n> ++\t\t\t\ti |= 0x01;\n> ++\t\t\t}\n> ++\t\t\tif (moff & 0x0000ff00) {\n> ++\t\t\t\tout[outpos++] = moff >> 8;\n> ++\t\t\t\ti |= 0x02;\n> ++\t\t\t}\n> ++\t\t\tif (moff & 0x00ff0000) {\n> ++\t\t\t\tout[outpos++] = moff >> 16;\n> ++\t\t\t\ti |= 0x04;\n> ++\t\t\t}\n> ++\t\t\tif (moff & 0xff000000) {\n> ++\t\t\t\tout[outpos++] = moff >> 24;\n> ++\t\t\t\ti |= 0x08;\n> ++\t\t\t}\n> ++\n> ++\t\t\tif (msize & 0x00ff) {\n> ++\t\t\t\tout[outpos++] = msize >> 0;\n> ++\t\t\t\ti |= 0x10;\n> ++\t\t\t}\n> ++\t\t\tif (msize & 0xff00) {\n> ++\t\t\t\tout[outpos++] = msize >> 8;\n> ++\t\t\t\ti |= 0x20;\n> ++\t\t\t}\n> \n> \t\t\t*op = i;\n> \n> 7: 9a6de12b807 ! 7: 1d0ce59cb68 wildmatch: explicitly mark intentional use of the comma operator\n> @@ Metadata\n> Author: Johannes Schindelin \n> \n> ## Commit message ##\n> - wildmatch: explicitly mark intentional use of the comma operator\n> + wildmatch: avoid using of the comma operator\n> \n> The comma operator is a somewhat obscure C feature that is often used by\n> mistake and can even cause unintentional code flow. That is why the\n> `-Wcomma` option of clang was introduced: To identify unintentional uses\n> of the comma operator.\n> \n> - To mark such a usage as intentional, the value needs to be cast to\n> - `void`, which we do here.\n> -\n> In this instance, the usage is intentional because it allows storing the\n> value of the current character as `prev_ch` before making the next\n> character the current one, all of which happens in the loop condition\n> that lets the loop stop at a closing bracket.\n> \n> - The alternative to using the comma operator would be to move those\n> + However, it is hard to read.\n> +\n> + The chosen alternative to using the comma operator is to move those\n> assignments from the condition into the loop body; In this particular\n> - case that would require the assignments to either be duplicated or to\n> - introduce and use a `goto` target before the assignments, though,\n> - because the loop body contains a `continue` for the case where a\n> - character class is found that starts with `[:` but does not end in `:]`\n> - (and the assignments should occur even when that code path is taken).\n> + case that requires special care because the loop body contains a\n> + `continue` for the case where a character class is found that starts\n> + with `[:` but does not end in `:]` (and the assignments should occur\n> + even when that code path is taken), which needs to be turned into a\n> + `goto`.\n> \n> + Helped-by: Phillip Wood \n> Signed-off-by: Johannes Schindelin \n> \n> ## wildmatch.c ##\n> +@@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n> + \t\t\t\t\t\tp_ch = '[';\n> + \t\t\t\t\t\tif (t_ch == p_ch)\n> + \t\t\t\t\t\t\tmatched = 1;\n> +-\t\t\t\t\t\tcontinue;\n> ++\t\t\t\t\t\tgoto next;\n> + \t\t\t\t\t}\n> + \t\t\t\t\tif (CC_EQ(s,i, \"alnum\")) {\n> + \t\t\t\t\t\tif (ISALNUM(t_ch))\n> @@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)\n> \t\t\t\t\tp_ch = 0; /* This makes \"prev_ch\" get set to 0. */\n> \t\t\t\t} else if (t_ch == p_ch)\n> \t\t\t\t\tmatched = 1;\n> -\t\t\t} while (prev_ch = p_ch, (p_ch = *++p) != ']');\n> -+\t\t\t} while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');\n> ++next:\n> ++\t\t\t\tprev_ch = p_ch;\n> ++\t\t\t\tp_ch = *++p;\n> ++\t\t\t} while (p_ch != ']');\n> \t\t\tif (matched == negated ||\n> \t\t\t ((flags & WM_PATHNAME) && t_ch == '/'))\n> \t\t\t\treturn WM_NOMATCH;\n> 8: dc626f36df3 ! 8: b8405f3d237 compat/regex: explicitly mark intentional use of the comma operator\n> @@ Commit message\n> \n> ## compat/regex/regex_internal.c ##\n> @@ compat/regex/regex_internal.c: re_node_set_merge (re_node_set *dest, const re_node_set *src)\n> - for (sbase = dest->nelem + 2 * src->nelem,\n> is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )\n> {\n> -- if (dest->elems[id] == src->elems[is])\n> + if (dest->elems[id] == src->elems[is])\n> -\tis--, id--;\n> -- else if (dest->elems[id] < src->elems[is])\n> -+ if (dest->elems[id] == src->elems[is]) {\n> -+\tis--;\n> -+\tid--;\n> -+ } else if (dest->elems[id] < src->elems[is])\n> ++\t{\n> ++\t is--;\n> ++\t id--;\n> ++\t}\n> + else if (dest->elems[id] < src->elems[is])\n> \tdest->elems[--sbase] = src->elems[is--];\n> else /* if (dest->elems[id] > src->elems[is]) */\n> - \t--id;\n> \n> ## compat/regex/regexec.c ##\n> @@ compat/regex/regexec.c: sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,\n> 9: 91f86c3aba9 ! 9: 6b6cd556465 clang: warn when the comma operator is used\n> @@ Commit message\n> warn about code using the comma operator (because it is typically\n> unintentional and wants to use the semicolon instead).\n> \n> + Helped-by: Patrick Steinhardt \n> Signed-off-by: Johannes Schindelin \n> \n> ## config.mak.dev ##\n> @@ config.mak.dev: DEVELOPER_CFLAGS += -Wvla\n> ifneq ($(filter clang4,$(COMPILER_FEATURES)),)\n> DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare\n> endif\n> +\n> + ## meson.build ##\n> +@@ meson.build: libgit_dependencies = [ ]\n> + # Makefile.\n> + if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'\n> + foreach cflag : [\n> ++ '-Wcomma',\n> + '-Wdeclaration-after-statement',\n> + '-Wformat-security',\n> + '-Wold-style-definition',\n> 10: 2f6f31240fe ! 10: 77f1dcaca1c detect-compiler: detect clang even if it found CUDA\n> @@ Commit message\n> Let's unconfuse the script by letting it parse the first matching line\n> and ignore the rest.\n> \n> + Helped-by: Eric Sunshine \n> Signed-off-by: Johannes Schindelin \n> \n> ## detect-compiler ##\n> @@ detect-compiler: CC=\"$*\"\n> # FreeBSD clang version 3.4.1 (tags/RELEASE...)\n> get_version_line() {\n> -\tLANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '\n> -+\tLANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'\n> ++\tLANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}'\n> }\n> \n> get_family() {\n> \n\n"},{"id":"515209","messageId":"Z+XIROew1umqs1XA@nand.local","threadId":"63188","inReplyTo":"1c8a3d80-5817-1e2a-de8c-06046d372fea@gmx.de","subject":"Re: [PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2025-03-27T21:51:00Z","receivedAt":"2025-03-27T21:51:03Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Mar 27, 2025 at 11:29:16AM +0100, Johannes Schindelin wrote:\n> Hi Taylor,\n>\n> On Wed, 26 Mar 2025, Taylor Blau wrote:\n>\n> > On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> > > diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c\n> > > index ec5cc5d2dd1..7672583bf7e 100644\n> > > --- a/compat/regex/regex_internal.c\n> > > +++ b/compat/regex/regex_internal.c\n> > > @@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)\n> > > for (sbase = dest->nelem + 2 * src->nelem,\n> > > is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )\n> > > {\n> > > - if (dest->elems[id] == src->elems[is])\n> > > -\tis--, id--;\n> > > - else if (dest->elems[id] < src->elems[is])\n> > > + if (dest->elems[id] == src->elems[is]) {\n> > > +\tis--;\n> > > +\tid--;\n> > > + } else if (dest->elems[id] < src->elems[is])\n> >\n> > Should the other arms of this conditional have matching curly-braces?\n>\n> No. Have a look around in that file, that's not the coding convention.\n\nI was just about to respond that even though it breaks the convention,\nthat we should encourage good hygeine by ensuring new code follows the\nCodingGuidelines, even if it looks wonky in the context of the rest of\nthe file.\n\nBut this is compat/regex code, which clearly does not need to follow the\nconvention. Sorry about that!\n\nThanks,\nTaylor\n"},{"id":"515258","messageId":"xmqqtt7chdqd.fsf@gitster.g","threadId":"63188","inReplyTo":"504e63df-77d2-4cd0-bdcc-bd9949d34ce5@gmail.com","subject":"Re: [PATCH v3 00/10] Avoid the comma operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-03-29T00:39:22Z","receivedAt":"2025-03-29T00:39:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Johannes\n>\n> On 27/03/2025 11:52, Johannes Schindelin via GitGitGadget wrote:\n>> Changes since v2:\n>> * Made the sed construct in detect-compiler portable (thanks, Eric\n>> Sunshine!)\n>> * The majority of the feedback disagreed with the more compact format in\n>> diff-delta.c, so I changed it to the long format (thanks, Phillip Wood!)\n>> * The more succinct and safer, but less readable, cast in the loop\n>> condition of the dowild() function was replaced with the goto-based\n>> alternative I had mentioned as a possibility in the commit message\n>> (thanks, Phillip Wood!)\n>> * I adjusted the style of my compat/regex/ patch to the surrounding code's.\n>> * The -Wcomma option is now used in Meson-based clang builds, too (thanks,\n>> Patrick Steinhardt!)\n>\n> The range-diff below looks good to me, thanks for making our code base\n> clearer.\n>\n> Best Wishes\n>\n> Phillip\n\nYup, thanks Dscho, and all who gave valuable input to polish the\nseries.\n\nWill queue.\n"}]}