* [PATCH v2 01/10] remote-curl: avoid using the comma operator unnecessarily
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-25 23:32 ` [PATCH v2 02/10] rebase: " Johannes Schindelin via GitGitGadget
` (11 subsequent siblings)
12 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote-curl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 1273507a96c..590b228f67f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads,
packet_buf_flush(&preamble);
memset(&rpc, 0, sizeof(rpc));
- rpc.service_name = "git-upload-pack",
+ rpc.service_name = "git-upload-pack";
rpc.gzip_request = 1;
err = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);
@@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)
packet_buf_flush(&preamble);
memset(&rpc, 0, sizeof(rpc));
- rpc.service_name = "git-receive-pack",
+ rpc.service_name = "git-receive-pack";
err = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);
if (rpc_result.len)
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 02/10] rebase: avoid using the comma operator unnecessarily
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
2025-03-25 23:32 ` [PATCH v2 01/10] remote-curl: avoid using the comma operator unnecessarily Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-25 23:32 ` [PATCH v2 03/10] kwset: " Johannes Schindelin via GitGitGadget
` (10 subsequent siblings)
12 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rebase.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index d4715ed35d7..62bdf7276f7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,
strbuf_addf(&msg, "%s (start): checkout %s",
options.reflog_action, options.onto_name);
ropts.oid = &options.onto->object.oid;
- ropts.orig_head = &options.orig_head->object.oid,
+ ropts.orig_head = &options.orig_head->object.oid;
ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 03/10] kwset: avoid using the comma operator unnecessarily
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
2025-03-25 23:32 ` [PATCH v2 01/10] remote-curl: avoid using the comma operator unnecessarily Johannes Schindelin via GitGitGadget
2025-03-25 23:32 ` [PATCH v2 02/10] rebase: " Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-25 23:32 ` [PATCH v2 04/10] clar: " Johannes Schindelin via GitGitGadget
` (9 subsequent siblings)
12 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
kwset.c | 54 +++++++++++++++++++++++++++++-------------------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/kwset.c b/kwset.c
index 1714eada608..064329434e5 100644
--- a/kwset.c
+++ b/kwset.c
@@ -197,10 +197,13 @@ kwsincr (kwset_t kws, char const *text, size_t len)
while (link && label != link->label)
{
links[depth] = link;
- if (label < link->label)
- dirs[depth++] = L, link = link->llink;
- else
- dirs[depth++] = R, link = link->rlink;
+ if (label < link->label) {
+ dirs[depth++] = L;
+ link = link->llink;
+ } else {
+ dirs[depth++] = R;
+ link = link->rlink;
+ }
}
/* The current character doesn't have an outgoing link at
@@ -257,14 +260,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)
switch (dirs[depth + 1])
{
case L:
- r = links[depth], t = r->llink, rl = t->rlink;
- t->rlink = r, r->llink = rl;
+ r = links[depth]; t = r->llink; rl = t->rlink;
+ t->rlink = r; r->llink = rl;
t->balance = r->balance = 0;
break;
case R:
- r = links[depth], l = r->llink, t = l->rlink;
- rl = t->rlink, lr = t->llink;
- t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;
+ r = links[depth]; l = r->llink; t = l->rlink;
+ rl = t->rlink; lr = t->llink;
+ t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;
l->balance = t->balance != 1 ? 0 : -1;
r->balance = t->balance != (char) -1 ? 0 : 1;
t->balance = 0;
@@ -277,14 +280,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)
switch (dirs[depth + 1])
{
case R:
- l = links[depth], t = l->rlink, lr = t->llink;
- t->llink = l, l->rlink = lr;
+ l = links[depth]; t = l->rlink; lr = t->llink;
+ t->llink = l; l->rlink = lr;
t->balance = l->balance = 0;
break;
case L:
- l = links[depth], r = l->rlink, t = r->llink;
- lr = t->llink, rl = t->rlink;
- t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;
+ l = links[depth]; r = l->rlink; t = r->llink;
+ lr = t->llink; rl = t->rlink;
+ t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;
l->balance = t->balance != 1 ? 0 : -1;
r->balance = t->balance != (char) -1 ? 0 : 1;
t->balance = 0;
@@ -567,22 +570,22 @@ bmexec (kwset_t kws, char const *text, size_t size)
{
while (tp <= ep)
{
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
if (d == 0)
goto found;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
if (d == 0)
goto found;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
if (d == 0)
goto found;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
}
break;
found:
@@ -649,7 +652,8 @@ cwexec (kwset_t kws, char const *text, size_t len, struct kwsmatch *kwsmatch)
mch = NULL;
else
{
- mch = text, accept = kwset->trie;
+ mch = text;
+ accept = kwset->trie;
goto match;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 04/10] clar: avoid using the comma operator unnecessarily
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 03/10] kwset: " Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-26 5:54 ` Patrick Steinhardt
2025-03-25 23:32 ` [PATCH v2 05/10] xdiff: " Johannes Schindelin via GitGitGadget
` (8 subsequent siblings)
12 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. In this instance, it
makes the code harder to read than necessary, too. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/unit-tests/clar/clar/fs.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/t/unit-tests/clar/clar/fs.h b/t/unit-tests/clar/clar/fs.h
index 8b206179fc4..2203743fb48 100644
--- a/t/unit-tests/clar/clar/fs.h
+++ b/t/unit-tests/clar/clar/fs.h
@@ -376,9 +376,12 @@ fs_copydir_helper(const char *source, const char *dest, int dest_mode)
mkdir(dest, dest_mode);
cl_assert_(source_dir = opendir(source), "Could not open source dir");
- while ((d = (errno = 0, readdir(source_dir))) != NULL) {
+ for (;;) {
char *child;
+ errno = 0;
+ if ((d = readdir(source_dir)) == NULL)
+ break;
if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
continue;
@@ -479,9 +482,12 @@ fs_rmdir_helper(const char *path)
struct dirent *d;
cl_assert_(dir = opendir(path), "Could not open dir");
- while ((d = (errno = 0, readdir(dir))) != NULL) {
+ for (;;) {
char *child;
+ errno = 0;
+ if ((d = readdir(dir)) == NULL)
+ break;
if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
continue;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 04/10] clar: avoid using the comma operator unnecessarily
2025-03-25 23:32 ` [PATCH v2 04/10] clar: " Johannes Schindelin via GitGitGadget
@ 2025-03-26 5:54 ` Patrick Steinhardt
2025-03-26 7:03 ` Johannes Schindelin
0 siblings, 1 reply; 64+ messages in thread
From: Patrick Steinhardt @ 2025-03-26 5:54 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Phillip Wood, Karthik Nayak, Jeff King,
Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:08PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The comma operator is a somewhat obscure C feature that is often used by
> mistake and can even cause unintentional code flow. In this instance, it
> makes the code harder to read than necessary, too. Better use a
> semicolon instead.
This code has changed upstream already, but let's roll with your change
anyway. I plan to update the clar to the upstream version soonish once I
have landed integer comparisons, and will take care that there aren't
any other comment operators left.
Patrick
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v2 04/10] clar: avoid using the comma operator unnecessarily
2025-03-26 5:54 ` Patrick Steinhardt
@ 2025-03-26 7:03 ` Johannes Schindelin
0 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin @ 2025-03-26 7:03 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Phillip Wood, Karthik Nayak, Jeff King
Hi Patrick,
On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> On Tue, Mar 25, 2025 at 11:32:08PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The comma operator is a somewhat obscure C feature that is often used by
> > mistake and can even cause unintentional code flow. In this instance, it
> > makes the code harder to read than necessary, too. Better use a
> > semicolon instead.
>
> This code has changed upstream already, but let's roll with your change
> anyway. I plan to update the clar to the upstream version soonish once I
> have landed integer comparisons, and will take care that there aren't
> any other comment operators left.
Thank you for putting so much energy into improving the tests, I really
appreciate it!
Ciao,
Johannes
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH v2 05/10] xdiff: avoid using the comma operator unnecessarily
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (3 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 04/10] clar: " Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-25 23:32 ` [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator Johannes Schindelin via GitGitGadget
` (7 subsequent siblings)
12 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. While the code in
this patch used the comma operator intentionally (to avoid curly
brackets around two statements, each, that want to be guarded by a
condition), it is better to surround it with curly brackets and to use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
xdiff/xdiffi.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 8889b8b62a1..5a96e36dfbe 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -211,8 +211,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
for (d = fmax; d >= fmin; d -= 2) {
i1 = XDL_MIN(kvdf[d], lim1);
i2 = i1 - d;
- if (lim2 < i2)
- i1 = lim2 + d, i2 = lim2;
+ if (lim2 < i2) {
+ i1 = lim2 + d;
+ i2 = lim2;
+ }
if (fbest < i1 + i2) {
fbest = i1 + i2;
fbest1 = i1;
@@ -223,8 +225,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
for (d = bmax; d >= bmin; d -= 2) {
i1 = XDL_MAX(off1, kvdb[d]);
i2 = i1 - d;
- if (i2 < off2)
- i1 = off2 + d, i2 = off2;
+ if (i2 < off2) {
+ i1 = off2 + d;
+ i2 = off2;
+ }
if (i1 + i2 < bbest) {
bbest = i1 + i2;
bbest1 = i1;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (4 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 05/10] xdiff: " Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-26 5:54 ` Patrick Steinhardt
2025-03-25 23:32 ` [PATCH v2 07/10] wildmatch: " Johannes Schindelin via GitGitGadget
` (6 subsequent siblings)
12 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
`-Wcomma` option of clang was introduced: To identify unintentional uses
of the comma operator.
Intentional uses include situations where one wants to avoid curly
brackets around multiple statements that need to be guarded by a
condition. This is the case here, as the repetitive nature of the
statements is easier to see for a human reader this way.
To mark this usage as intentional, the return value of the statement
before the comma needs to be cast to `void`, which we do here.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diff-delta.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/diff-delta.c b/diff-delta.c
index a4faf73829b..a03ba10b2be 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -439,18 +439,18 @@ create_delta(const struct delta_index *index,
i = 0x80;
if (moff & 0x000000ff)
- out[outpos++] = moff >> 0, i |= 0x01;
+ (void)(out[outpos++] = moff >> 0), i |= 0x01;
if (moff & 0x0000ff00)
- out[outpos++] = moff >> 8, i |= 0x02;
+ (void)(out[outpos++] = moff >> 8), i |= 0x02;
if (moff & 0x00ff0000)
- out[outpos++] = moff >> 16, i |= 0x04;
+ (void)(out[outpos++] = moff >> 16), i |= 0x04;
if (moff & 0xff000000)
- out[outpos++] = moff >> 24, i |= 0x08;
+ (void)(out[outpos++] = moff >> 24), i |= 0x08;
if (msize & 0x00ff)
- out[outpos++] = msize >> 0, i |= 0x10;
+ (void)(out[outpos++] = msize >> 0), i |= 0x10;
if (msize & 0xff00)
- out[outpos++] = msize >> 8, i |= 0x20;
+ (void)(out[outpos++] = msize >> 8), i |= 0x20;
*op = i;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator Johannes Schindelin via GitGitGadget
@ 2025-03-26 5:54 ` Patrick Steinhardt
2025-03-26 7:20 ` Johannes Schindelin
0 siblings, 1 reply; 64+ messages in thread
From: Patrick Steinhardt @ 2025-03-26 5:54 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Phillip Wood, Karthik Nayak, Jeff King,
Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:10PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The comma operator is a somewhat obscure C feature that is often used by
> mistake and can even cause unintentional code flow. That is why the
> `-Wcomma` option of clang was introduced: To identify unintentional uses
> of the comma operator.
>
> Intentional uses include situations where one wants to avoid curly
> brackets around multiple statements that need to be guarded by a
> condition. This is the case here, as the repetitive nature of the
> statements is easier to see for a human reader this way.
>
> To mark this usage as intentional, the return value of the statement
> before the comma needs to be cast to `void`, which we do here.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff-delta.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/diff-delta.c b/diff-delta.c
> index a4faf73829b..a03ba10b2be 100644
> --- a/diff-delta.c
> +++ b/diff-delta.c
> @@ -439,18 +439,18 @@ create_delta(const struct delta_index *index,
> i = 0x80;
>
> if (moff & 0x000000ff)
> - out[outpos++] = moff >> 0, i |= 0x01;
> + (void)(out[outpos++] = moff >> 0), i |= 0x01;
> if (moff & 0x0000ff00)
> - out[outpos++] = moff >> 8, i |= 0x02;
> + (void)(out[outpos++] = moff >> 8), i |= 0x02;
> if (moff & 0x00ff0000)
> - out[outpos++] = moff >> 16, i |= 0x04;
> + (void)(out[outpos++] = moff >> 16), i |= 0x04;
> if (moff & 0xff000000)
> - out[outpos++] = moff >> 24, i |= 0x08;
> + (void)(out[outpos++] = moff >> 24), i |= 0x08;
>
> if (msize & 0x00ff)
> - out[outpos++] = msize >> 0, i |= 0x10;
> + (void)(out[outpos++] = msize >> 0), i |= 0x10;
> if (msize & 0xff00)
> - out[outpos++] = msize >> 8, i |= 0x20;
> + (void)(out[outpos++] = msize >> 8), i |= 0x20;
Hm. I think the end result is even more confusing than before. Why don't
we introduce curly braces here, same as in preceding commits?
Patrick
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator
2025-03-26 5:54 ` Patrick Steinhardt
@ 2025-03-26 7:20 ` Johannes Schindelin
2025-03-26 10:17 ` Phillip Wood
0 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin @ 2025-03-26 7:20 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Phillip Wood, Karthik Nayak, Jeff King
Hi Patrick,
On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> On Tue, Mar 25, 2025 at 11:32:10PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > The comma operator is a somewhat obscure C feature that is often used by
> > mistake and can even cause unintentional code flow. That is why the
> > `-Wcomma` option of clang was introduced: To identify unintentional uses
> > of the comma operator.
> >
> > Intentional uses include situations where one wants to avoid curly
> > brackets around multiple statements that need to be guarded by a
> > condition. This is the case here, as the repetitive nature of the
> > statements is easier to see for a human reader this way.
> >
> > To mark this usage as intentional, the return value of the statement
> > before the comma needs to be cast to `void`, which we do here.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff-delta.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/diff-delta.c b/diff-delta.c
> > index a4faf73829b..a03ba10b2be 100644
> > --- a/diff-delta.c
> > +++ b/diff-delta.c
> > @@ -439,18 +439,18 @@ create_delta(const struct delta_index *index,
> > i = 0x80;
> >
> > if (moff & 0x000000ff)
> > - out[outpos++] = moff >> 0, i |= 0x01;
> > + (void)(out[outpos++] = moff >> 0), i |= 0x01;
> > if (moff & 0x0000ff00)
> > - out[outpos++] = moff >> 8, i |= 0x02;
> > + (void)(out[outpos++] = moff >> 8), i |= 0x02;
> > if (moff & 0x00ff0000)
> > - out[outpos++] = moff >> 16, i |= 0x04;
> > + (void)(out[outpos++] = moff >> 16), i |= 0x04;
> > if (moff & 0xff000000)
> > - out[outpos++] = moff >> 24, i |= 0x08;
> > + (void)(out[outpos++] = moff >> 24), i |= 0x08;
> >
> > if (msize & 0x00ff)
> > - out[outpos++] = msize >> 0, i |= 0x10;
> > + (void)(out[outpos++] = msize >> 0), i |= 0x10;
> > if (msize & 0xff00)
> > - out[outpos++] = msize >> 8, i |= 0x20;
> > + (void)(out[outpos++] = msize >> 8), i |= 0x20;
>
> Hm. I think the end result is even more confusing than before. Why don't
> we introduce curly braces here, same as in preceding commits?
The interleaved -/+ lines make it admittedly hard to see what I meant.
I'll unwind it a bit (presenting only the `moff` part, the same
consideration applies to `msize`):
if (moff & 0x000000ff)
(void)(out[outpos++] = moff >> 0), i |= 0x01;
if (moff & 0x0000ff00)
(void)(out[outpos++] = moff >> 8), i |= 0x02;
if (moff & 0x00ff0000)
(void)(out[outpos++] = moff >> 16), i |= 0x04;
if (moff & 0xff000000)
(void)(out[outpos++] = moff >> 24), i |= 0x08;
In this form, it is very obvious (from comparing the right-side half of
the lines) that a shifted version of `moff` is appended to `out` and `i`
gets a bit set, and the correlation between shift width and the set bit
is relatively easy to see and validate (at least my brain has an easy time
here, thanks to the alignment and thanks to visual similarity between the
non-blank parts).
It is admittedly quite a bit harder not to be distracted by the repetitive
`(void)(out[...` parts to understand and validate the `if` conditions on
the left-hand side, but thanks to those repetitive parts being identical,
and being only one line between those `if` lines, I can bring my brain to
focus only on the differences of the bitmask and understand and verify
them with relatively little effort.
When I compared this form to the following, the cognitive load to wrap my
head around the code is quite a bit higher there:
if (moff & 0x000000ff) {
out[outpos++] = moff >> 0;
i |= 0x01;
}
if (moff & 0x0000ff00) {
out[outpos++] = moff >> 8;
i |= 0x02;
}
if (moff & 0x00ff0000) {
out[outpos++] = moff >> 16;
i |= 0x04;
}
if (moff & 0xff000000) {
out[outpos++] = moff >> 24;
i |= 0x08;
}
The reason is the visual distance between the near-identical code.
Having said that, I do realize that my brain quite possibly works in
special ways here and that the general preference is to go with the latter
form.
Do you have a strong opinion which form to use?
Ciao,
Johannes
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator
2025-03-26 7:20 ` Johannes Schindelin
@ 2025-03-26 10:17 ` Phillip Wood
2025-03-26 20:33 ` Taylor Blau
0 siblings, 1 reply; 64+ messages in thread
From: Phillip Wood @ 2025-03-26 10:17 UTC (permalink / raw)
To: Johannes Schindelin, Patrick Steinhardt
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Karthik Nayak, Jeff King
Hi Johannes
On 26/03/2025 07:20, Johannes Schindelin wrote:
> Hi Patrick,
> On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
>> Hm. I think the end result is even more confusing than before. Why don't
>> we introduce curly braces here, same as in preceding commits?
>
> The interleaved -/+ lines make it admittedly hard to see what I meant.
> I'll unwind it a bit (presenting only the `moff` part, the same
> consideration applies to `msize`):
>
> if (moff & 0x000000ff)
> (void)(out[outpos++] = moff >> 0), i |= 0x01;
> if (moff & 0x0000ff00)
> (void)(out[outpos++] = moff >> 8), i |= 0x02;
> if (moff & 0x00ff0000)
> (void)(out[outpos++] = moff >> 16), i |= 0x04;
> if (moff & 0xff000000)
> (void)(out[outpos++] = moff >> 24), i |= 0x08;
>
> In this form, it is very obvious (from comparing the right-side half of
> the lines) that a shifted version of `moff` is appended to `out` and `i`
> gets a bit set, and the correlation between shift width and the set bit
> is relatively easy to see and validate (at least my brain has an easy time
> here, thanks to the alignment and thanks to visual similarity between the
> non-blank parts).
>
> It is admittedly quite a bit harder not to be distracted by the repetitive
> `(void)(out[...` parts to understand and validate the `if` conditions on
> the left-hand side,
That makes it pretty unreadable for me. If you're worried about the
vertical space we could perhaps keep both statements on a single line so
that we're only adding a single newline for the closing brace rather
than two.
Best Wishes
Phillip
but thanks to those repetitive parts being identical,
> and being only one line between those `if` lines, I can bring my brain to
> focus only on the differences of the bitmask and understand and verify
> them with relatively little effort.
>
> When I compared this form to the following, the cognitive load to wrap my
> head around the code is quite a bit higher there:
>
> if (moff & 0x000000ff) {
> out[outpos++] = moff >> 0;
> i |= 0x01;
> }
> if (moff & 0x0000ff00) {
> out[outpos++] = moff >> 8;
> i |= 0x02;
> }
> if (moff & 0x00ff0000) {
> out[outpos++] = moff >> 16;
> i |= 0x04;
> }
> if (moff & 0xff000000) {
> out[outpos++] = moff >> 24;
> i |= 0x08;
> }
>
> The reason is the visual distance between the near-identical code.
>
> Having said that, I do realize that my brain quite possibly works in
> special ways here and that the general preference is to go with the latter
> form.
>
> Do you have a strong opinion which form to use?
>
> Ciao,
> Johannes
>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator
2025-03-26 10:17 ` Phillip Wood
@ 2025-03-26 20:33 ` Taylor Blau
2025-03-27 1:31 ` Chris Torek
0 siblings, 1 reply; 64+ messages in thread
From: Taylor Blau @ 2025-03-26 20:33 UTC (permalink / raw)
To: phillip.wood
Cc: Johannes Schindelin, Patrick Steinhardt,
Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Karthik Nayak, Jeff King
On Wed, Mar 26, 2025 at 10:17:50AM +0000, Phillip Wood wrote:
> Hi Johannes
>
> On 26/03/2025 07:20, Johannes Schindelin wrote:
> > Hi Patrick,
> > On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> > > Hm. I think the end result is even more confusing than before. Why don't
> > > we introduce curly braces here, same as in preceding commits?
> >
> > The interleaved -/+ lines make it admittedly hard to see what I meant.
> > I'll unwind it a bit (presenting only the `moff` part, the same
> > consideration applies to `msize`):
> >
> > if (moff & 0x000000ff)
> > (void)(out[outpos++] = moff >> 0), i |= 0x01;
> > if (moff & 0x0000ff00)
> > (void)(out[outpos++] = moff >> 8), i |= 0x02;
> > if (moff & 0x00ff0000)
> > (void)(out[outpos++] = moff >> 16), i |= 0x04;
> > if (moff & 0xff000000)
> > (void)(out[outpos++] = moff >> 24), i |= 0x08;
> >
> > In this form, it is very obvious (from comparing the right-side half of
> > the lines) that a shifted version of `moff` is appended to `out` and `i`
> > gets a bit set, and the correlation between shift width and the set bit
> > is relatively easy to see and validate (at least my brain has an easy time
> > here, thanks to the alignment and thanks to visual similarity between the
> > non-blank parts).
> >
> > It is admittedly quite a bit harder not to be distracted by the repetitive
> > `(void)(out[...` parts to understand and validate the `if` conditions on
> > the left-hand side,
>
> That makes it pretty unreadable for me. If you're worried about the vertical
> space we could perhaps keep both statements on a single line so that we're
> only adding a single newline for the closing brace rather than two.
My $.02 here would be that the form:
if (moff & 0x000000ff) {
out[outpos++] = moff >> 0;
i |= 0x01;
}
is the most readable, and fits within the conventions of our
CodingGuidelines. I don't really love the idea of writing:
if (moff & 0x000000ff) {
out[outpos++] = moff >> 0); i |= 0x01;
}
, since it looks like a single-line if-statement and is thus tempting to
drop the braces, which would make the code incorrect, as 'i |= 0x01'
would be executed unconditionally. I suppose you could write
if (moff & 0x000000ff) {
out[outpos++] = moff >> 0); i |= 0x01; }
, but that feels even uglier to me ;-).
Thanks,
Taylor
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator
2025-03-26 20:33 ` Taylor Blau
@ 2025-03-27 1:31 ` Chris Torek
0 siblings, 0 replies; 64+ messages in thread
From: Chris Torek @ 2025-03-27 1:31 UTC (permalink / raw)
To: Taylor Blau
Cc: phillip.wood, Johannes Schindelin, Patrick Steinhardt,
Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Karthik Nayak, Jeff King
(Just picking the latest message to reply to, not really quite right)
> > > if (moff & 0x000000ff)
> > > (void)(out[outpos++] = moff >> 0), i |= 0x01;
> > > if (moff & 0x0000ff00)
> > > (void)(out[outpos++] = moff >> 8), i |= 0x02;
> > > if (moff & 0x00ff0000)
> > > (void)(out[outpos++] = moff >> 16), i |= 0x04;
> > > if (moff & 0xff000000)
> > > (void)(out[outpos++] = moff >> 24), i |= 0x08;
Might be overkill but:
#define XXX(index) do { \
if (moff & (0xffUL << ((index) * 8))) {
out[outpos++] = moff >> ((index) * 8);
i |= 1 << (index);
}
} while (0)
XXX(0);
XXX(1);
XXX(2);
XXX(3);
#undef XXX
would do the trick. Pick a proper name for XXX of course.
Chris
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (5 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 06/10] diff-delta: explicitly mark intentional use of the comma operator Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-26 5:54 ` Patrick Steinhardt
` (2 more replies)
2025-03-25 23:32 ` [PATCH v2 08/10] compat/regex: " Johannes Schindelin via GitGitGadget
` (5 subsequent siblings)
12 siblings, 3 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
`-Wcomma` option of clang was introduced: To identify unintentional uses
of the comma operator.
To mark such a usage as intentional, the value needs to be cast to
`void`, which we do here.
In this instance, the usage is intentional because it allows storing the
value of the current character as `prev_ch` before making the next
character the current one, all of which happens in the loop condition
that lets the loop stop at a closing bracket.
The alternative to using the comma operator would be to move those
assignments from the condition into the loop body; In this particular
case that would require the assignments to either be duplicated or to
introduce and use a `goto` target before the assignments, though,
because the loop body contains a `continue` for the case where a
character class is found that starts with `[:` but does not end in `:]`
(and the assignments should occur even when that code path is taken).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wildmatch.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/wildmatch.c b/wildmatch.c
index 8ea29141bd7..ce8108a6d57 100644
--- a/wildmatch.c
+++ b/wildmatch.c
@@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
p_ch = 0; /* This makes "prev_ch" get set to 0. */
} else if (t_ch == p_ch)
matched = 1;
- } while (prev_ch = p_ch, (p_ch = *++p) != ']');
+ } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
if (matched == negated ||
((flags & WM_PATHNAME) && t_ch == '/'))
return WM_NOMATCH;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 07/10] wildmatch: " Johannes Schindelin via GitGitGadget
@ 2025-03-26 5:54 ` Patrick Steinhardt
2025-03-26 7:46 ` Johannes Schindelin
2025-03-26 7:49 ` Junio C Hamano
2025-03-26 10:14 ` Phillip Wood
2 siblings, 1 reply; 64+ messages in thread
From: Patrick Steinhardt @ 2025-03-26 5:54 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Phillip Wood, Karthik Nayak, Jeff King,
Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:11PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/wildmatch.c b/wildmatch.c
> index 8ea29141bd7..ce8108a6d57 100644
> --- a/wildmatch.c
> +++ b/wildmatch.c
> @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> p_ch = 0; /* This makes "prev_ch" get set to 0. */
> } else if (t_ch == p_ch)
> matched = 1;
> - } while (prev_ch = p_ch, (p_ch = *++p) != ']');
> + } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
> if (matched == negated ||
> ((flags & WM_PATHNAME) && t_ch == '/'))
> return WM_NOMATCH;
In this case I agree that it makes sense to not introduce curly braces
for brevity.
Patrick
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator
2025-03-26 5:54 ` Patrick Steinhardt
@ 2025-03-26 7:46 ` Johannes Schindelin
0 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin @ 2025-03-26 7:46 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Phillip Wood, Karthik Nayak, Jeff King
Hi Patrick,
On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> On Tue, Mar 25, 2025 at 11:32:11PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/wildmatch.c b/wildmatch.c
> > index 8ea29141bd7..ce8108a6d57 100644
> > --- a/wildmatch.c
> > +++ b/wildmatch.c
> > @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> > p_ch = 0; /* This makes "prev_ch" get set to 0. */
> > } else if (t_ch == p_ch)
> > matched = 1;
> > - } while (prev_ch = p_ch, (p_ch = *++p) != ']');
> > + } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
> > if (matched == negated ||
> > ((flags & WM_PATHNAME) && t_ch == '/'))
> > return WM_NOMATCH;
>
> In this case I agree that it makes sense to not introduce curly braces
> for brevity.
I should probably have mentioned that this patch took the longest to write
of the entire patch series, by far. Not because of the changed code, that
was easy. No, when I wrote the commit message and spotted the `continue`,
I did not want to leave it at that because the code around that `continue`
looks... well, let's just say that it could be rewritten for clarity.
In fact, when I looked at the following part, I was immediately
_convinced_ that it is incorrect, and had to work very hard to understand
why it works correctly, even going so far as to single-step through a
couple of examples, e.g. `test-tool wildmatch wildmatch 'b' '[[:a-z]'`:
} else if (p_ch == '[' && p[1] == ':') {
const uchar *s;
int i;
for (s = p += 2; (p_ch = *p) && p_ch != ']'; p++) {} /*SHARED ITERATOR*/
if (!p_ch)
return WM_ABORT_ALL;
i = p - s - 1;
if (i < 0 || p[-1] != ':') {
/* Didn't find ":]", so treat like a normal set. */
p = s - 2;
p_ch = '[';
if (t_ch == p_ch)
matched = 1;
continue;
}
For context, here is a link:
https://gitlab.com/gitlab-org/git/-/blob/v2.49.0/wildmatch.c?ref_type=tags#L213-227.
At this point, `t_ch` is the current character in the text to match;
`p_ch` (and `*p`) is the current character in the _pattern_, and it is
_inside_ a `[...]` character range, and it wants to match a character
class of the form `[:alnum:]` but also allow for a regular character range
that starts by including the colon. And that latter scenario, where it is
_not_ a special character class, is what this `continue` is all about.
What threw me was that `t_ch == p_ch` condition _directly_ after assigning
`p_ch = '['`. It is still a pattern I would always immediately suspect to
be a bug: Why not compare `t_ch == '['` instead, which would be much more
obvious?
You will also note that the value of `i` is only used in the condition,
and it is basically used to determine whether the the colon was
immediately followed by the closing bracket or not, which could be
rewritten to be a lot more obvious.
So what does the `continue` do here? It skips back to the outer loop,
continuing with the `:` as next pattern character in the character range,
and for that it is crucial that the `p_ch` be set to the opening bracket
and `p` is rewound _just_ so that the assignments in the loop condition
can set things up for the next loop iteration (still within that `case
'['`) not to be thrown by that `[:`.
I probably did a terrible job explaining why the code works as intended,
but I'll just chalk it up to the contortions my brain had to exercise to
understand that code.
But all that's safely outside the scope of the question whether to use a
comma operator or not ;-)
Ciao,
Johannes
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 07/10] wildmatch: " Johannes Schindelin via GitGitGadget
2025-03-26 5:54 ` Patrick Steinhardt
@ 2025-03-26 7:49 ` Junio C Hamano
2025-03-26 10:14 ` Phillip Wood
2 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2025-03-26 7:49 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Patrick Steinhardt, Phillip Wood,
Karthik Nayak, Jeff King, Johannes Schindelin
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> diff --git a/wildmatch.c b/wildmatch.c
> index 8ea29141bd7..ce8108a6d57 100644
> --- a/wildmatch.c
> +++ b/wildmatch.c
> @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> p_ch = 0; /* This makes "prev_ch" get set to 0. */
> } else if (t_ch == p_ch)
> matched = 1;
> - } while (prev_ch = p_ch, (p_ch = *++p) != ']');
> + } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
> if (matched == negated ||
> ((flags & WM_PATHNAME) && t_ch == '/'))
> return WM_NOMATCH;
Hmph, I personally do not find the (void) convention any easier to
read and understand than the original, and more importantly, it does
not look like a good signal to say that the author knows what they
are doing.
For this particular loop, it probably makes a lot more sense to do a
stupid and more obvious rewrite. And the same thing can be said for
the previous step.
Perhaps like so?
diff-delta.c | 37 ++++++++++++++++++++++++-------------
wildmatch.c | 4 +++-
2 files changed, 27 insertions(+), 14 deletions(-)
diff --git c/diff-delta.c w/diff-delta.c
index a4faf73829..a999b875d4 100644
--- c/diff-delta.c
+++ w/diff-delta.c
@@ -438,19 +438,30 @@ create_delta(const struct delta_index *index,
op = out + outpos++;
i = 0x80;
- if (moff & 0x000000ff)
- out[outpos++] = moff >> 0, i |= 0x01;
- if (moff & 0x0000ff00)
- out[outpos++] = moff >> 8, i |= 0x02;
- if (moff & 0x00ff0000)
- out[outpos++] = moff >> 16, i |= 0x04;
- if (moff & 0xff000000)
- out[outpos++] = moff >> 24, i |= 0x08;
-
- if (msize & 0x00ff)
- out[outpos++] = msize >> 0, i |= 0x10;
- if (msize & 0xff00)
- out[outpos++] = msize >> 8, i |= 0x20;
+ if (moff & 0x000000ff) {
+ out[outpos++] = moff >> 0;
+ i |= 0x01;
+ }
+ if (moff & 0x0000ff00) {
+ out[outpos++] = moff >> 8;
+ i |= 0x02;
+ }
+ if (moff & 0x00ff0000) {
+ out[outpos++] = moff >> 16;
+ i |= 0x04;
+ }
+ if (moff & 0xff000000) {
+ out[outpos++] = moff >> 24;
+ i |= 0x08;
+ }
+ if (msize & 0x00ff) {
+ out[outpos++] = msize >> 0;
+ i |= 0x10;
+ }
+ if (msize & 0xff00) {
+ out[outpos++] = msize >> 8;
+ i |= 0x20;
+ }
*op = i;
diff --git c/wildmatch.c w/wildmatch.c
index 8ea29141bd..29f4b4df75 100644
--- c/wildmatch.c
+++ w/wildmatch.c
@@ -268,7 +268,9 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
p_ch = 0; /* This makes "prev_ch" get set to 0. */
} else if (t_ch == p_ch)
matched = 1;
- } while (prev_ch = p_ch, (p_ch = *++p) != ']');
+
+ prev_ch = p_ch;
+ } while ((p_ch = *++p) != ']');
if (matched == negated ||
((flags & WM_PATHNAME) && t_ch == '/'))
return WM_NOMATCH;
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 07/10] wildmatch: " Johannes Schindelin via GitGitGadget
2025-03-26 5:54 ` Patrick Steinhardt
2025-03-26 7:49 ` Junio C Hamano
@ 2025-03-26 10:14 ` Phillip Wood
2025-03-26 10:34 ` Junio C Hamano
2 siblings, 1 reply; 64+ messages in thread
From: Phillip Wood @ 2025-03-26 10:14 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git
Cc: Philip Oakley, Patrick Steinhardt, Karthik Nayak, Jeff King,
Johannes Schindelin
Hi Johannes
On 25/03/2025 23:32, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The alternative to using the comma operator would be to move those
> assignments from the condition into the loop body; In this particular
> case that would require the assignments to either be duplicated or to
> introduce and use a `goto` target before the assignments, though,
> because the loop body contains a `continue` for the case where a
> character class is found that starts with `[:` but does not end in `:]`
> (and the assignments should occur even when that code path is taken).
I think that would be clearer, something like the diff below
Best Wishes
Phillip
---- >8 ----
diff --git a/wildmatch.c b/wildmatch.c
index 8ea29141bd7..7230544c356 100644
--- a/wildmatch.c
+++ b/wildmatch.c
@@ -223,7 +223,7 @@
p_ch = '[';
if (t_ch == p_ch)
matched = 1;
- continue;
+ goto next;
}
if (CC_EQ(s,i, "alnum")) {
if (ISALNUM(t_ch))
@@ -268,7 +268,10 @@
p_ch = 0; /* This makes "prev_ch" get set to 0. */
} else if (t_ch == p_ch)
matched = 1;
- } while (prev_ch = p_ch, (p_ch = *++p) != ']');
+ next:
+ prev_ch = p_ch;
+ p_ch = *++p;
+ } while (p_ch != ']');
if (matched == negated ||
((flags & WM_PATHNAME) && t_ch == '/'))
return WM_NOMATCH;
Phillip> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> wildmatch.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/wildmatch.c b/wildmatch.c
> index 8ea29141bd7..ce8108a6d57 100644
> --- a/wildmatch.c
> +++ b/wildmatch.c
> @@ -268,7 +268,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> p_ch = 0; /* This makes "prev_ch" get set to 0. */
> } else if (t_ch == p_ch)
> matched = 1;
> - } while (prev_ch = p_ch, (p_ch = *++p) != ']');
> + } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
> if (matched == negated ||
> ((flags & WM_PATHNAME) && t_ch == '/'))
> return WM_NOMATCH;
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 07/10] wildmatch: explicitly mark intentional use of the comma operator
2025-03-26 10:14 ` Phillip Wood
@ 2025-03-26 10:34 ` Junio C Hamano
0 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2025-03-26 10:34 UTC (permalink / raw)
To: Phillip Wood
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Karthik Nayak, Jeff King, Johannes Schindelin
Phillip Wood <phillip.wood123@gmail.com> writes:
> I think that would be clearer, something like the diff below
>
> Best Wishes
>
> Phillip
>
> ---- >8 ----
>
> diff --git a/wildmatch.c b/wildmatch.c
> index 8ea29141bd7..7230544c356 100644
> --- a/wildmatch.c
> +++ b/wildmatch.c
> @@ -223,7 +223,7 @@
> p_ch = '[';
> if (t_ch == p_ch)
> matched = 1;
> - continue;
> + goto next;
> }
> if (CC_EQ(s,i, "alnum")) {
> if (ISALNUM(t_ch))
> @@ -268,7 +268,10 @@
> p_ch = 0; /* This makes "prev_ch" get set to 0. */
> } else if (t_ch == p_ch)
> matched = 1;
> - } while (prev_ch = p_ch, (p_ch = *++p) != ']');
> + next:
> + prev_ch = p_ch;
> + p_ch = *++p;
> + } while (p_ch != ']');
> if (matched == negated ||
> ((flags & WM_PATHNAME) && t_ch == '/'))
> return WM_NOMATCH;
>
Ah, I missed that "continue"; with the "next:" label, it looks quite
clear what is going on.
I like it. Thanks.
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (6 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 07/10] wildmatch: " Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-26 20:35 ` Taylor Blau
2025-03-25 23:32 ` [PATCH v2 09/10] clang: warn when the comma operator is used Johannes Schindelin via GitGitGadget
` (4 subsequent siblings)
12 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
`-Wcomma` option of clang was introduced: To identify unintentional uses
of the comma operator.
In the `compat/regex/` code, the comma operator is used twice, once to
avoid surrounding two conditional statements with curly brackets, the
other one to increment two counters simultaneously in a `do ... while`
condition.
The first one is replaced with a proper conditional block, surrounded by
curly brackets.
The second one would be harder to replace because the loop contains two
`continue`s. Therefore, the second one is marked as intentional by
casting the value-to-discard to `void`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/regex/regex_internal.c | 7 ++++---
compat/regex/regexec.c | 2 +-
2 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
index ec5cc5d2dd1..7672583bf7e 100644
--- a/compat/regex/regex_internal.c
+++ b/compat/regex/regex_internal.c
@@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)
for (sbase = dest->nelem + 2 * src->nelem,
is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
{
- if (dest->elems[id] == src->elems[is])
- is--, id--;
- else if (dest->elems[id] < src->elems[is])
+ if (dest->elems[id] == src->elems[is]) {
+ is--;
+ id--;
+ } else if (dest->elems[id] < src->elems[is])
dest->elems[--sbase] = src->elems[is--];
else /* if (dest->elems[id] > src->elems[is]) */
--id;
diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
index 2eeec82f407..c08f1bbe1f5 100644
--- a/compat/regex/regexec.c
+++ b/compat/regex/regexec.c
@@ -2210,7 +2210,7 @@ sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,
/* mctx->bkref_ents may have changed, reload the pointer. */
entry = mctx->bkref_ents + enabled_idx;
}
- while (enabled_idx++, entry++->more);
+ while ((void)enabled_idx++, entry++->more);
}
err = REG_NOERROR;
free_return:
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator
2025-03-25 23:32 ` [PATCH v2 08/10] compat/regex: " Johannes Schindelin via GitGitGadget
@ 2025-03-26 20:35 ` Taylor Blau
2025-03-27 10:29 ` Johannes Schindelin
0 siblings, 1 reply; 64+ messages in thread
From: Taylor Blau @ 2025-03-26 20:35 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Patrick Steinhardt, Phillip Wood,
Karthik Nayak, Jeff King, Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
> index ec5cc5d2dd1..7672583bf7e 100644
> --- a/compat/regex/regex_internal.c
> +++ b/compat/regex/regex_internal.c
> @@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)
> for (sbase = dest->nelem + 2 * src->nelem,
> is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
> {
> - if (dest->elems[id] == src->elems[is])
> - is--, id--;
> - else if (dest->elems[id] < src->elems[is])
> + if (dest->elems[id] == src->elems[is]) {
> + is--;
> + id--;
> + } else if (dest->elems[id] < src->elems[is])
Should the other arms of this conditional have matching curly-braces?
Thanks,
Taylor
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator
2025-03-26 20:35 ` Taylor Blau
@ 2025-03-27 10:29 ` Johannes Schindelin
2025-03-27 21:51 ` Taylor Blau
0 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin @ 2025-03-27 10:29 UTC (permalink / raw)
To: Taylor Blau
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Phillip Wood, Karthik Nayak, Jeff King
Hi Taylor,
On Wed, 26 Mar 2025, Taylor Blau wrote:
> On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
> > index ec5cc5d2dd1..7672583bf7e 100644
> > --- a/compat/regex/regex_internal.c
> > +++ b/compat/regex/regex_internal.c
> > @@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)
> > for (sbase = dest->nelem + 2 * src->nelem,
> > is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
> > {
> > - if (dest->elems[id] == src->elems[is])
> > - is--, id--;
> > - else if (dest->elems[id] < src->elems[is])
> > + if (dest->elems[id] == src->elems[is]) {
> > + is--;
> > + id--;
> > + } else if (dest->elems[id] < src->elems[is])
>
> Should the other arms of this conditional have matching curly-braces?
No. Have a look around in that file, that's not the coding convention.
However, a valid concern is that I used Git's convention for the curly
brackets (appending the open bracket to the `if` line), which is _not_ the
convention used in this file, which uses GNU conventions instead. I will
address that concern in the next iteration.
Ciao,
Johannes
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 08/10] compat/regex: explicitly mark intentional use of the comma operator
2025-03-27 10:29 ` Johannes Schindelin
@ 2025-03-27 21:51 ` Taylor Blau
0 siblings, 0 replies; 64+ messages in thread
From: Taylor Blau @ 2025-03-27 21:51 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Phillip Wood, Karthik Nayak, Jeff King
On Thu, Mar 27, 2025 at 11:29:16AM +0100, Johannes Schindelin wrote:
> Hi Taylor,
>
> On Wed, 26 Mar 2025, Taylor Blau wrote:
>
> > On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
> > > index ec5cc5d2dd1..7672583bf7e 100644
> > > --- a/compat/regex/regex_internal.c
> > > +++ b/compat/regex/regex_internal.c
> > > @@ -1231,9 +1231,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)
> > > for (sbase = dest->nelem + 2 * src->nelem,
> > > is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
> > > {
> > > - if (dest->elems[id] == src->elems[is])
> > > - is--, id--;
> > > - else if (dest->elems[id] < src->elems[is])
> > > + if (dest->elems[id] == src->elems[is]) {
> > > + is--;
> > > + id--;
> > > + } else if (dest->elems[id] < src->elems[is])
> >
> > Should the other arms of this conditional have matching curly-braces?
>
> No. Have a look around in that file, that's not the coding convention.
I was just about to respond that even though it breaks the convention,
that we should encourage good hygeine by ensuring new code follows the
CodingGuidelines, even if it looks wonky in the context of the rest of
the file.
But this is compat/regex code, which clearly does not need to follow the
convention. Sorry about that!
Thanks,
Taylor
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH v2 09/10] clang: warn when the comma operator is used
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (7 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 08/10] compat/regex: " Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-26 5:54 ` Patrick Steinhardt
2025-03-25 23:32 ` [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA Johannes Schindelin via GitGitGadget
` (3 subsequent siblings)
12 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When compiling Git using `clang`, the `-Wcomma` option can be used to
warn about code using the comma operator (because it is typically
unintentional and wants to use the semicolon instead).
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.mak.dev | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/config.mak.dev b/config.mak.dev
index 0fd8cc4d355..31423638169 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla
DEVELOPER_CFLAGS += -Wwrite-strings
DEVELOPER_CFLAGS += -fno-common
+ifneq ($(filter clang9,$(COMPILER_FEATURES)),)
+DEVELOPER_CFLAGS += -Wcomma
+endif
+
ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
endif
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 09/10] clang: warn when the comma operator is used
2025-03-25 23:32 ` [PATCH v2 09/10] clang: warn when the comma operator is used Johannes Schindelin via GitGitGadget
@ 2025-03-26 5:54 ` Patrick Steinhardt
2025-03-26 7:50 ` Johannes Schindelin
0 siblings, 1 reply; 64+ messages in thread
From: Patrick Steinhardt @ 2025-03-26 5:54 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Phillip Wood, Karthik Nayak, Jeff King,
Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:13PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> When compiling Git using `clang`, the `-Wcomma` option can be used to
> warn about code using the comma operator (because it is typically
> unintentional and wants to use the semicolon instead).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> config.mak.dev | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/config.mak.dev b/config.mak.dev
> index 0fd8cc4d355..31423638169 100644
> --- a/config.mak.dev
> +++ b/config.mak.dev
> @@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla
> DEVELOPER_CFLAGS += -Wwrite-strings
> DEVELOPER_CFLAGS += -fno-common
>
> +ifneq ($(filter clang9,$(COMPILER_FEATURES)),)
> +DEVELOPER_CFLAGS += -Wcomma
> +endif
> +
> ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
> DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
> endif
Let's squash the below diff into this commit. The loop already makes
sure that the compiler supports the flag, so there is no need to special
case Clang.
Patrick
diff --git a/meson.build b/meson.build
index dd231b669b6..a7658d62ea0 100644
--- a/meson.build
+++ b/meson.build
@@ -717,6 +717,7 @@ libgit_dependencies = [ ]
# Makefile.
if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'
foreach cflag : [
+ '-Wcomma',
'-Wdeclaration-after-statement',
'-Wformat-security',
'-Wold-style-definition',
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 09/10] clang: warn when the comma operator is used
2025-03-26 5:54 ` Patrick Steinhardt
@ 2025-03-26 7:50 ` Johannes Schindelin
2025-03-26 8:33 ` Patrick Steinhardt
0 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin @ 2025-03-26 7:50 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Phillip Wood, Karthik Nayak, Jeff King
Hi Patrick,
On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> On Tue, Mar 25, 2025 at 11:32:13PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When compiling Git using `clang`, the `-Wcomma` option can be used to
> > warn about code using the comma operator (because it is typically
> > unintentional and wants to use the semicolon instead).
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > config.mak.dev | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/config.mak.dev b/config.mak.dev
> > index 0fd8cc4d355..31423638169 100644
> > --- a/config.mak.dev
> > +++ b/config.mak.dev
> > @@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla
> > DEVELOPER_CFLAGS += -Wwrite-strings
> > DEVELOPER_CFLAGS += -fno-common
> >
> > +ifneq ($(filter clang9,$(COMPILER_FEATURES)),)
> > +DEVELOPER_CFLAGS += -Wcomma
> > +endif
> > +
> > ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
> > DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
> > endif
>
> Let's squash the below diff into this commit. The loop already makes
> sure that the compiler supports the flag, so there is no need to special
> case Clang.
Okay, will do.
For my curiosity...
> diff --git a/meson.build b/meson.build
> index dd231b669b6..a7658d62ea0 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -717,6 +717,7 @@ libgit_dependencies = [ ]
> # Makefile.
> if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'
This `get_argument_syntax() == 'gcc'` condition catches `clang`? What
other compilers that aren't GCC does it catch?
Ciao,
Johannes
> foreach cflag : [
> + '-Wcomma',
> '-Wdeclaration-after-statement',
> '-Wformat-security',
> '-Wold-style-definition',
>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 09/10] clang: warn when the comma operator is used
2025-03-26 7:50 ` Johannes Schindelin
@ 2025-03-26 8:33 ` Patrick Steinhardt
2025-03-27 10:18 ` Johannes Schindelin
0 siblings, 1 reply; 64+ messages in thread
From: Patrick Steinhardt @ 2025-03-26 8:33 UTC (permalink / raw)
To: Johannes Schindelin
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Phillip Wood, Karthik Nayak, Jeff King
On Wed, Mar 26, 2025 at 08:50:24AM +0100, Johannes Schindelin wrote:
> On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> > diff --git a/meson.build b/meson.build
> > index dd231b669b6..a7658d62ea0 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -717,6 +717,7 @@ libgit_dependencies = [ ]
> > # Makefile.
> > if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'
>
> This `get_argument_syntax() == 'gcc'` condition catches `clang`? What
> other compilers that aren't GCC does it catch?
Yes, it does catch Clang as well as the Intel compiler. The list of
compilers and their respective syntax can be found at [1]
Patrick
[1]: https://mesonbuild.com/Reference-tables.html#compiler-ids
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 09/10] clang: warn when the comma operator is used
2025-03-26 8:33 ` Patrick Steinhardt
@ 2025-03-27 10:18 ` Johannes Schindelin
0 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin @ 2025-03-27 10:18 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Phillip Wood, Karthik Nayak, Jeff King
Hi Patrick,
On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> On Wed, Mar 26, 2025 at 08:50:24AM +0100, Johannes Schindelin wrote:
> > On Wed, 26 Mar 2025, Patrick Steinhardt wrote:
> > > diff --git a/meson.build b/meson.build
> > > index dd231b669b6..a7658d62ea0 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -717,6 +717,7 @@ libgit_dependencies = [ ]
> > > # Makefile.
> > > if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'
> >
> > This `get_argument_syntax() == 'gcc'` condition catches `clang`? What
> > other compilers that aren't GCC does it catch?
>
> Yes, it does catch Clang as well as the Intel compiler. The list of
> compilers and their respective syntax can be found at [1]
>
> [1]: https://mesonbuild.com/Reference-tables.html#compiler-ids
Thank you for this valuable reference!
Ciao,
Johannes
^ permalink raw reply [flat|nested] 64+ messages in thread
* [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (8 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 09/10] clang: warn when the comma operator is used Johannes Schindelin via GitGitGadget
@ 2025-03-25 23:32 ` Johannes Schindelin via GitGitGadget
2025-03-26 17:41 ` Jeff King
2025-03-26 5:54 ` [PATCH v2 00/10] Avoid the comma operator Patrick Steinhardt
` (2 subsequent siblings)
12 siblings, 1 reply; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-25 23:32 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In my setup, clang finds `/usr/local/cuda` and hence the output of
`clang -v` ends with this line:
Found CUDA installation: /usr/local/cuda, version
This confuses the `detect-compiler` script because it matches _all_
lines that contain the needle "version" surrounded by spaces. As a
consequence, the `get_family` function returns two lines: "Ubuntu clang"
and above-mentioned line, which the `case` statement does not handle
well and hence reports "unknown compiler family" instead of the expected
set of "clang14", "clang13", ..., "clang1" output.
Let's unconfuse the script by letting it parse the first matching line
and ignore the rest.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
detect-compiler | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/detect-compiler b/detect-compiler
index a87650b71bb..01eca3a781d 100755
--- a/detect-compiler
+++ b/detect-compiler
@@ -9,7 +9,7 @@ CC="$*"
#
# FreeBSD clang version 3.4.1 (tags/RELEASE...)
get_version_line() {
- LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
+ LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
}
get_family() {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA
2025-03-25 23:32 ` [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA Johannes Schindelin via GitGitGadget
@ 2025-03-26 17:41 ` Jeff King
2025-03-26 18:07 ` Eric Sunshine
0 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2025-03-26 17:41 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Patrick Steinhardt, Phillip Wood,
Karthik Nayak, Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:
> In my setup, clang finds `/usr/local/cuda` and hence the output of
> `clang -v` ends with this line:
>
> Found CUDA installation: /usr/local/cuda, version
>
> This confuses the `detect-compiler` script because it matches _all_
> lines that contain the needle "version" surrounded by spaces. As a
> consequence, the `get_family` function returns two lines: "Ubuntu clang"
> and above-mentioned line, which the `case` statement does not handle
> well and hence reports "unknown compiler family" instead of the expected
> set of "clang14", "clang13", ..., "clang1" output.
>
> Let's unconfuse the script by letting it parse the first matching line
> and ignore the rest.
Makes sense. I wondered if this:
> get_version_line() {
> - LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
> + LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
might be more readable with "grep -m1", but it looks like "-m" is not in
POSIX. So what you wrote is probably safer.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA
2025-03-26 17:41 ` Jeff King
@ 2025-03-26 18:07 ` Eric Sunshine
2025-03-27 5:14 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: Eric Sunshine @ 2025-03-26 18:07 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Johannes Schindelin
On Wed, Mar 26, 2025 at 1:44 PM Jeff King <peff@peff.net> wrote:
> On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > Let's unconfuse the script by letting it parse the first matching line
> > and ignore the rest.
>
> Makes sense. I wondered if this:
>
> > get_version_line() {
> > - LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
> > + LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
>
> might be more readable with "grep -m1", but it looks like "-m" is not in
> POSIX. So what you wrote is probably safer.
It's probably an indication that I've done too much `sed` programming,
but I find Dscho's version more obvious. That aside, your response
made me take a closer look at what Dscho wrote and I noticed that it
is syntactically flawed, at least for BSD-lineage `sed`. Testing on
macOS reveals that this is indeed so:
% LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'
sed: 1: "/ version /{p;q}": extra characters at the end of q command
The problem is that the `q` function takes no arguments, but
BSD-lineage `sed` thinks that the closing `}` is an argument rather
than a terminator. Fixing this requires inserting a terminator after
`q`, which will be either a newline character or a semicolon. So, the
correct form is:
sed -n '/ version /{p;q;}
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA
2025-03-26 18:07 ` Eric Sunshine
@ 2025-03-27 5:14 ` Jeff King
2025-03-27 5:21 ` Eric Sunshine
0 siblings, 1 reply; 64+ messages in thread
From: Jeff King @ 2025-03-27 5:14 UTC (permalink / raw)
To: Eric Sunshine
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Johannes Schindelin
On Wed, Mar 26, 2025 at 02:07:10PM -0400, Eric Sunshine wrote:
> On Wed, Mar 26, 2025 at 1:44 PM Jeff King <peff@peff.net> wrote:
> > On Tue, Mar 25, 2025 at 11:32:14PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > Let's unconfuse the script by letting it parse the first matching line
> > > and ignore the rest.
> >
> > Makes sense. I wondered if this:
> >
> > > get_version_line() {
> > > - LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
> > > + LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
> >
> > might be more readable with "grep -m1", but it looks like "-m" is not in
> > POSIX. So what you wrote is probably safer.
>
> It's probably an indication that I've done too much `sed` programming,
> but I find Dscho's version more obvious. That aside, your response
> made me take a closer look at what Dscho wrote and I noticed that it
> is syntactically flawed, at least for BSD-lineage `sed`. Testing on
> macOS reveals that this is indeed so:
>
> % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'
> sed: 1: "/ version /{p;q}": extra characters at the end of q command
>
> The problem is that the `q` function takes no arguments, but
> BSD-lineage `sed` thinks that the closing `}` is an argument rather
> than a terminator. Fixing this requires inserting a terminator after
> `q`, which will be either a newline character or a semicolon. So, the
> correct form is:
>
> sed -n '/ version /{p;q;}
Heh, I think it was the braces and semicolons that made my spider-sense
tingle, probably because I've been bitten by those subtleties in the
past.
I think just "/foo/p;q" works on GNU sed, but no idea if it does
elsewhere. What you wrote seems the safest.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA
2025-03-27 5:14 ` Jeff King
@ 2025-03-27 5:21 ` Eric Sunshine
2025-03-27 6:35 ` Jeff King
0 siblings, 1 reply; 64+ messages in thread
From: Eric Sunshine @ 2025-03-27 5:21 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Johannes Schindelin
On Thu, Mar 27, 2025 at 1:14 AM Jeff King <peff@peff.net> wrote:
> On Wed, Mar 26, 2025 at 02:07:10PM -0400, Eric Sunshine wrote:
> > It's probably an indication that I've done too much `sed` programming,
> > but I find Dscho's version more obvious. That aside, your response
> > made me take a closer look at what Dscho wrote and I noticed that it
> > is syntactically flawed, at least for BSD-lineage `sed`. Testing on
> > macOS reveals that this is indeed so:
> >
> > % LANG=C LC_ALL=C cc -v 2>&1 | sed -n '/ version /{p;q}'
> > sed: 1: "/ version /{p;q}": extra characters at the end of q command
> >
> > The problem is that the `q` function takes no arguments, but
> > BSD-lineage `sed` thinks that the closing `}` is an argument rather
> > than a terminator. Fixing this requires inserting a terminator after
> > `q`, which will be either a newline character or a semicolon. So, the
> > correct form is:
> >
> > sed -n '/ version /{p;q;}
>
> Heh, I think it was the braces and semicolons that made my spider-sense
> tingle, probably because I've been bitten by those subtleties in the
> past.
>
> I think just "/foo/p;q" works on GNU sed, but no idea if it does
> elsewhere. What you wrote seems the safest.
That's not quite the same, though. The patternless `q` will cause
`sed` to terminate upon reading the first line of input, not upon the
first line which contains " version ". This matters, for instance, if
the first line output by `$CC -v` is not the version string (i.e. it
might be a copyright notice).
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA
2025-03-27 5:21 ` Eric Sunshine
@ 2025-03-27 6:35 ` Jeff King
0 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2025-03-27 6:35 UTC (permalink / raw)
To: Eric Sunshine
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Johannes Schindelin
On Thu, Mar 27, 2025 at 01:21:03AM -0400, Eric Sunshine wrote:
> > I think just "/foo/p;q" works on GNU sed, but no idea if it does
> > elsewhere. What you wrote seems the safest.
>
> That's not quite the same, though. The patternless `q` will cause
> `sed` to terminate upon reading the first line of input, not upon the
> first line which contains " version ". This matters, for instance, if
> the first line output by `$CC -v` is not the version string (i.e. it
> might be a copyright notice).
Oh, duh, you're right. I even tested to make sure were still quitting
after matching, but of course that is because we were quitting even when
_not_ matching.
For some reason I also thought /foo/pq would work, but it doesn't. I
wonder if it used to under some implementations, or if I'm simply going
senile.
(The point still remains that what you wrote is the correct thing).
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread
* Re: [PATCH v2 00/10] Avoid the comma operator
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (9 preceding siblings ...)
2025-03-25 23:32 ` [PATCH v2 10/10] detect-compiler: detect clang even if it found CUDA Johannes Schindelin via GitGitGadget
@ 2025-03-26 5:54 ` Patrick Steinhardt
2025-03-26 17:50 ` Jeff King
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
12 siblings, 0 replies; 64+ messages in thread
From: Patrick Steinhardt @ 2025-03-26 5:54 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Phillip Wood, Karthik Nayak, Jeff King,
Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:
> The comma operator
> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is
> rarely used in C anymore, and typically indicates a typo. Just like in these
> instances, where a semicolon was meant to be used, as there is no need to
> discard the first statement's result here.
>
> Changes since v1:
>
> * Use -Wcomma when compiling with clang and with DEVELOPER=1.
> * Address the remaining instances pointed out by clang (and by Phillip).
Thanks for all of these fixes!
Patrick
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v2 00/10] Avoid the comma operator
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (10 preceding siblings ...)
2025-03-26 5:54 ` [PATCH v2 00/10] Avoid the comma operator Patrick Steinhardt
@ 2025-03-26 17:50 ` Jeff King
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
12 siblings, 0 replies; 64+ messages in thread
From: Jeff King @ 2025-03-26 17:50 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget
Cc: git, Philip Oakley, Patrick Steinhardt, Phillip Wood,
Karthik Nayak, Johannes Schindelin
On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:
> The comma operator
> [https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is
> rarely used in C anymore, and typically indicates a typo. Just like in these
> instances, where a semicolon was meant to be used, as there is no need to
> discard the first statement's result here.
>
> Changes since v1:
>
> * Use -Wcomma when compiling with clang and with DEVELOPER=1.
> * Address the remaining instances pointed out by clang (and by Phillip).
Thanks for fixing these. I checked the diff against the quick-and-dirty
patch I posted earlier in the thread, and your resolutions for the
"easy" cases look good (though like others, I'd prefer switching to
semicolons for the one in diff-delta.c).
For the harder cases inside while() conditionals, the rewrites all look
correct to me. I do think that getting rid of the commas often makes the
result easier to read, but the discussion on wildmatch shows that it's
easy to get the transformation wrong. So I'd be happy enough slapping a
"(void)" on that one and moving on with our lives. The goal is not to
prettify that code (which was not even written for Git in the first
place) but to silence -Wcomma false positives so that we can find the
actual problems.
-Peff
^ permalink raw reply [flat|nested] 64+ messages in thread* [PATCH v3 00/10] Avoid the comma operator
2025-03-25 23:32 ` [PATCH v2 00/10] " Johannes Schindelin via GitGitGadget
` (11 preceding siblings ...)
2025-03-26 17:50 ` Jeff King
@ 2025-03-27 11:52 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 01/10] remote-curl: avoid using the comma operator unnecessarily Johannes Schindelin via GitGitGadget
` (10 more replies)
12 siblings, 11 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:52 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin
The comma operator
[https://en.cppreference.com/w/c/language/operator_other#Comma_operator] is
rarely used in C anymore, and typically indicates a typo. Just like in these
instances, where a semicolon was meant to be used, as there is no need to
discard the first statement's result here.
Changes since v2:
* Made the sed construct in detect-compiler portable (thanks, Eric
Sunshine!)
* The majority of the feedback disagreed with the more compact format in
diff-delta.c, so I changed it to the long format (thanks, Phillip Wood!)
* The more succinct and safer, but less readable, cast in the loop
condition of the dowild() function was replaced with the goto-based
alternative I had mentioned as a possibility in the commit message
(thanks, Phillip Wood!)
* I adjusted the style of my compat/regex/ patch to the surrounding code's.
* The -Wcomma option is now used in Meson-based clang builds, too (thanks,
Patrick Steinhardt!)
Changes since v1:
* Use -Wcomma when compiling with clang and with DEVELOPER=1.
* Address the remaining instances pointed out by clang (and by Phillip).
Johannes Schindelin (10):
remote-curl: avoid using the comma operator unnecessarily
rebase: avoid using the comma operator unnecessarily
kwset: avoid using the comma operator unnecessarily
clar: avoid using the comma operator unnecessarily
xdiff: avoid using the comma operator unnecessarily
diff-delta: avoid using the comma operator
wildmatch: avoid using of the comma operator
compat/regex: explicitly mark intentional use of the comma operator
clang: warn when the comma operator is used
detect-compiler: detect clang even if it found CUDA
builtin/rebase.c | 2 +-
compat/regex/regex_internal.c | 5 +++-
compat/regex/regexec.c | 2 +-
config.mak.dev | 4 +++
detect-compiler | 2 +-
diff-delta.c | 38 +++++++++++++++---------
kwset.c | 54 +++++++++++++++++++----------------
meson.build | 1 +
remote-curl.c | 4 +--
t/unit-tests/clar/clar/fs.h | 10 +++++--
wildmatch.c | 7 +++--
xdiff/xdiffi.c | 12 +++++---
12 files changed, 89 insertions(+), 52 deletions(-)
base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1889
Range-diff vs v2:
1: 913c7a0d296 = 1: 913c7a0d296 remote-curl: avoid using the comma operator unnecessarily
2: 37ff88b8275 = 2: 37ff88b8275 rebase: avoid using the comma operator unnecessarily
3: f601f4e74a5 = 3: f601f4e74a5 kwset: avoid using the comma operator unnecessarily
4: f60ebe376e1 = 4: f60ebe376e1 clar: avoid using the comma operator unnecessarily
5: 7239078413f = 5: 7239078413f xdiff: avoid using the comma operator unnecessarily
6: 5e0e8325620 ! 6: 045d695d73e diff-delta: explicitly mark intentional use of the comma operator
@@ Metadata
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
## Commit message ##
- diff-delta: explicitly mark intentional use of the comma operator
+ diff-delta: avoid using the comma operator
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
@@ Commit message
Intentional uses include situations where one wants to avoid curly
brackets around multiple statements that need to be guarded by a
condition. This is the case here, as the repetitive nature of the
- statements is easier to see for a human reader this way.
+ statements is easier to see for a human reader this way. At least in my
+ opinion.
- To mark this usage as intentional, the return value of the statement
- before the comma needs to be cast to `void`, which we do here.
+ However, opinions on this differ wildly, take 10 people and you have 10
+ different preferences.
+ On the Git mailing list, it seems that the consensus is to use the long
+ form instead, so let's do just that.
+
+ Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## diff-delta.c ##
@@ diff-delta.c: create_delta(const struct delta_index *index,
+ op = out + outpos++;
i = 0x80;
- if (moff & 0x000000ff)
+- if (moff & 0x000000ff)
- out[outpos++] = moff >> 0, i |= 0x01;
-+ (void)(out[outpos++] = moff >> 0), i |= 0x01;
- if (moff & 0x0000ff00)
+- if (moff & 0x0000ff00)
- out[outpos++] = moff >> 8, i |= 0x02;
-+ (void)(out[outpos++] = moff >> 8), i |= 0x02;
- if (moff & 0x00ff0000)
+- if (moff & 0x00ff0000)
- out[outpos++] = moff >> 16, i |= 0x04;
-+ (void)(out[outpos++] = moff >> 16), i |= 0x04;
- if (moff & 0xff000000)
+- if (moff & 0xff000000)
- out[outpos++] = moff >> 24, i |= 0x08;
-+ (void)(out[outpos++] = moff >> 24), i |= 0x08;
-
- if (msize & 0x00ff)
+-
+- if (msize & 0x00ff)
- out[outpos++] = msize >> 0, i |= 0x10;
-+ (void)(out[outpos++] = msize >> 0), i |= 0x10;
- if (msize & 0xff00)
+- if (msize & 0xff00)
- out[outpos++] = msize >> 8, i |= 0x20;
-+ (void)(out[outpos++] = msize >> 8), i |= 0x20;
++ if (moff & 0x000000ff) {
++ out[outpos++] = moff >> 0;
++ i |= 0x01;
++ }
++ if (moff & 0x0000ff00) {
++ out[outpos++] = moff >> 8;
++ i |= 0x02;
++ }
++ if (moff & 0x00ff0000) {
++ out[outpos++] = moff >> 16;
++ i |= 0x04;
++ }
++ if (moff & 0xff000000) {
++ out[outpos++] = moff >> 24;
++ i |= 0x08;
++ }
++
++ if (msize & 0x00ff) {
++ out[outpos++] = msize >> 0;
++ i |= 0x10;
++ }
++ if (msize & 0xff00) {
++ out[outpos++] = msize >> 8;
++ i |= 0x20;
++ }
*op = i;
7: 9a6de12b807 ! 7: 1d0ce59cb68 wildmatch: explicitly mark intentional use of the comma operator
@@ Metadata
Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
## Commit message ##
- wildmatch: explicitly mark intentional use of the comma operator
+ wildmatch: avoid using of the comma operator
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
`-Wcomma` option of clang was introduced: To identify unintentional uses
of the comma operator.
- To mark such a usage as intentional, the value needs to be cast to
- `void`, which we do here.
-
In this instance, the usage is intentional because it allows storing the
value of the current character as `prev_ch` before making the next
character the current one, all of which happens in the loop condition
that lets the loop stop at a closing bracket.
- The alternative to using the comma operator would be to move those
+ However, it is hard to read.
+
+ The chosen alternative to using the comma operator is to move those
assignments from the condition into the loop body; In this particular
- case that would require the assignments to either be duplicated or to
- introduce and use a `goto` target before the assignments, though,
- because the loop body contains a `continue` for the case where a
- character class is found that starts with `[:` but does not end in `:]`
- (and the assignments should occur even when that code path is taken).
+ case that requires special care because the loop body contains a
+ `continue` for the case where a character class is found that starts
+ with `[:` but does not end in `:]` (and the assignments should occur
+ even when that code path is taken), which needs to be turned into a
+ `goto`.
+ Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## wildmatch.c ##
+@@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)
+ p_ch = '[';
+ if (t_ch == p_ch)
+ matched = 1;
+- continue;
++ goto next;
+ }
+ if (CC_EQ(s,i, "alnum")) {
+ if (ISALNUM(t_ch))
@@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)
p_ch = 0; /* This makes "prev_ch" get set to 0. */
} else if (t_ch == p_ch)
matched = 1;
- } while (prev_ch = p_ch, (p_ch = *++p) != ']');
-+ } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
++next:
++ prev_ch = p_ch;
++ p_ch = *++p;
++ } while (p_ch != ']');
if (matched == negated ||
((flags & WM_PATHNAME) && t_ch == '/'))
return WM_NOMATCH;
8: dc626f36df3 ! 8: b8405f3d237 compat/regex: explicitly mark intentional use of the comma operator
@@ Commit message
## compat/regex/regex_internal.c ##
@@ compat/regex/regex_internal.c: re_node_set_merge (re_node_set *dest, const re_node_set *src)
- for (sbase = dest->nelem + 2 * src->nelem,
is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
{
-- if (dest->elems[id] == src->elems[is])
+ if (dest->elems[id] == src->elems[is])
- is--, id--;
-- else if (dest->elems[id] < src->elems[is])
-+ if (dest->elems[id] == src->elems[is]) {
-+ is--;
-+ id--;
-+ } else if (dest->elems[id] < src->elems[is])
++ {
++ is--;
++ id--;
++ }
+ else if (dest->elems[id] < src->elems[is])
dest->elems[--sbase] = src->elems[is--];
else /* if (dest->elems[id] > src->elems[is]) */
- --id;
## compat/regex/regexec.c ##
@@ compat/regex/regexec.c: sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,
9: 91f86c3aba9 ! 9: 6b6cd556465 clang: warn when the comma operator is used
@@ Commit message
warn about code using the comma operator (because it is typically
unintentional and wants to use the semicolon instead).
+ Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## config.mak.dev ##
@@ config.mak.dev: DEVELOPER_CFLAGS += -Wvla
ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
endif
+
+ ## meson.build ##
+@@ meson.build: libgit_dependencies = [ ]
+ # Makefile.
+ if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'
+ foreach cflag : [
++ '-Wcomma',
+ '-Wdeclaration-after-statement',
+ '-Wformat-security',
+ '-Wold-style-definition',
10: 2f6f31240fe ! 10: 77f1dcaca1c detect-compiler: detect clang even if it found CUDA
@@ Commit message
Let's unconfuse the script by letting it parse the first matching line
and ignore the rest.
+ Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
## detect-compiler ##
@@ detect-compiler: CC="$*"
# FreeBSD clang version 3.4.1 (tags/RELEASE...)
get_version_line() {
- LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
-+ LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
++ LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}'
}
get_family() {
--
gitgitgadget
^ permalink raw reply [flat|nested] 64+ messages in thread* [PATCH v3 01/10] remote-curl: avoid using the comma operator unnecessarily
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:52 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 02/10] rebase: " Johannes Schindelin via GitGitGadget
` (9 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:52 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
remote-curl.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 1273507a96c..590b228f67f 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1239,7 +1239,7 @@ static int fetch_git(struct discovery *heads,
packet_buf_flush(&preamble);
memset(&rpc, 0, sizeof(rpc));
- rpc.service_name = "git-upload-pack",
+ rpc.service_name = "git-upload-pack";
rpc.gzip_request = 1;
err = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);
@@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)
packet_buf_flush(&preamble);
memset(&rpc, 0, sizeof(rpc));
- rpc.service_name = "git-receive-pack",
+ rpc.service_name = "git-receive-pack";
err = rpc_service(&rpc, heads, args.v, &preamble, &rpc_result);
if (rpc_result.len)
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 02/10] rebase: avoid using the comma operator unnecessarily
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 01/10] remote-curl: avoid using the comma operator unnecessarily Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:52 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 03/10] kwset: " Johannes Schindelin via GitGitGadget
` (8 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:52 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
builtin/rebase.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index d4715ed35d7..62bdf7276f7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,
strbuf_addf(&msg, "%s (start): checkout %s",
options.reflog_action, options.onto_name);
ropts.oid = &options.onto->object.oid;
- ropts.orig_head = &options.orig_head->object.oid,
+ ropts.orig_head = &options.orig_head->object.oid;
ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
ropts.head_msg = msg.buf;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 03/10] kwset: avoid using the comma operator unnecessarily
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 01/10] remote-curl: avoid using the comma operator unnecessarily Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 02/10] rebase: " Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:52 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 04/10] clar: " Johannes Schindelin via GitGitGadget
` (7 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:52 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
kwset.c | 54 +++++++++++++++++++++++++++++-------------------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/kwset.c b/kwset.c
index 1714eada608..064329434e5 100644
--- a/kwset.c
+++ b/kwset.c
@@ -197,10 +197,13 @@ kwsincr (kwset_t kws, char const *text, size_t len)
while (link && label != link->label)
{
links[depth] = link;
- if (label < link->label)
- dirs[depth++] = L, link = link->llink;
- else
- dirs[depth++] = R, link = link->rlink;
+ if (label < link->label) {
+ dirs[depth++] = L;
+ link = link->llink;
+ } else {
+ dirs[depth++] = R;
+ link = link->rlink;
+ }
}
/* The current character doesn't have an outgoing link at
@@ -257,14 +260,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)
switch (dirs[depth + 1])
{
case L:
- r = links[depth], t = r->llink, rl = t->rlink;
- t->rlink = r, r->llink = rl;
+ r = links[depth]; t = r->llink; rl = t->rlink;
+ t->rlink = r; r->llink = rl;
t->balance = r->balance = 0;
break;
case R:
- r = links[depth], l = r->llink, t = l->rlink;
- rl = t->rlink, lr = t->llink;
- t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;
+ r = links[depth]; l = r->llink; t = l->rlink;
+ rl = t->rlink; lr = t->llink;
+ t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;
l->balance = t->balance != 1 ? 0 : -1;
r->balance = t->balance != (char) -1 ? 0 : 1;
t->balance = 0;
@@ -277,14 +280,14 @@ kwsincr (kwset_t kws, char const *text, size_t len)
switch (dirs[depth + 1])
{
case R:
- l = links[depth], t = l->rlink, lr = t->llink;
- t->llink = l, l->rlink = lr;
+ l = links[depth]; t = l->rlink; lr = t->llink;
+ t->llink = l; l->rlink = lr;
t->balance = l->balance = 0;
break;
case L:
- l = links[depth], r = l->rlink, t = r->llink;
- lr = t->llink, rl = t->rlink;
- t->llink = l, l->rlink = lr, t->rlink = r, r->llink = rl;
+ l = links[depth]; r = l->rlink; t = r->llink;
+ lr = t->llink; rl = t->rlink;
+ t->llink = l; l->rlink = lr; t->rlink = r; r->llink = rl;
l->balance = t->balance != 1 ? 0 : -1;
r->balance = t->balance != (char) -1 ? 0 : 1;
t->balance = 0;
@@ -567,22 +570,22 @@ bmexec (kwset_t kws, char const *text, size_t size)
{
while (tp <= ep)
{
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
if (d == 0)
goto found;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
if (d == 0)
goto found;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
if (d == 0)
goto found;
- d = d1[U(tp[-1])], tp += d;
- d = d1[U(tp[-1])], tp += d;
+ d = d1[U(tp[-1])]; tp += d;
+ d = d1[U(tp[-1])]; tp += d;
}
break;
found:
@@ -649,7 +652,8 @@ cwexec (kwset_t kws, char const *text, size_t len, struct kwsmatch *kwsmatch)
mch = NULL;
else
{
- mch = text, accept = kwset->trie;
+ mch = text;
+ accept = kwset->trie;
goto match;
}
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 04/10] clar: avoid using the comma operator unnecessarily
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (2 preceding siblings ...)
2025-03-27 11:52 ` [PATCH v3 03/10] kwset: " Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:52 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 05/10] xdiff: " Johannes Schindelin via GitGitGadget
` (6 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:52 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. In this instance, it
makes the code harder to read than necessary, too. Better use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
t/unit-tests/clar/clar/fs.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/t/unit-tests/clar/clar/fs.h b/t/unit-tests/clar/clar/fs.h
index 8b206179fc4..2203743fb48 100644
--- a/t/unit-tests/clar/clar/fs.h
+++ b/t/unit-tests/clar/clar/fs.h
@@ -376,9 +376,12 @@ fs_copydir_helper(const char *source, const char *dest, int dest_mode)
mkdir(dest, dest_mode);
cl_assert_(source_dir = opendir(source), "Could not open source dir");
- while ((d = (errno = 0, readdir(source_dir))) != NULL) {
+ for (;;) {
char *child;
+ errno = 0;
+ if ((d = readdir(source_dir)) == NULL)
+ break;
if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
continue;
@@ -479,9 +482,12 @@ fs_rmdir_helper(const char *path)
struct dirent *d;
cl_assert_(dir = opendir(path), "Could not open dir");
- while ((d = (errno = 0, readdir(dir))) != NULL) {
+ for (;;) {
char *child;
+ errno = 0;
+ if ((d = readdir(dir)) == NULL)
+ break;
if (!strcmp(d->d_name, ".") || !strcmp(d->d_name, ".."))
continue;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 05/10] xdiff: avoid using the comma operator unnecessarily
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (3 preceding siblings ...)
2025-03-27 11:52 ` [PATCH v3 04/10] clar: " Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:52 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:52 ` [PATCH v3 06/10] diff-delta: avoid using the comma operator Johannes Schindelin via GitGitGadget
` (5 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:52 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. While the code in
this patch used the comma operator intentionally (to avoid curly
brackets around two statements, each, that want to be guarded by a
condition), it is better to surround it with curly brackets and to use a
semicolon instead.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
xdiff/xdiffi.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 8889b8b62a1..5a96e36dfbe 100644
--- a/xdiff/xdiffi.c
+++ b/xdiff/xdiffi.c
@@ -211,8 +211,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
for (d = fmax; d >= fmin; d -= 2) {
i1 = XDL_MIN(kvdf[d], lim1);
i2 = i1 - d;
- if (lim2 < i2)
- i1 = lim2 + d, i2 = lim2;
+ if (lim2 < i2) {
+ i1 = lim2 + d;
+ i2 = lim2;
+ }
if (fbest < i1 + i2) {
fbest = i1 + i2;
fbest1 = i1;
@@ -223,8 +225,10 @@ static long xdl_split(unsigned long const *ha1, long off1, long lim1,
for (d = bmax; d >= bmin; d -= 2) {
i1 = XDL_MAX(off1, kvdb[d]);
i2 = i1 - d;
- if (i2 < off2)
- i1 = off2 + d, i2 = off2;
+ if (i2 < off2) {
+ i1 = off2 + d;
+ i2 = off2;
+ }
if (i1 + i2 < bbest) {
bbest = i1 + i2;
bbest1 = i1;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 06/10] diff-delta: avoid using the comma operator
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (4 preceding siblings ...)
2025-03-27 11:52 ` [PATCH v3 05/10] xdiff: " Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:52 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:53 ` [PATCH v3 07/10] wildmatch: avoid using of " Johannes Schindelin via GitGitGadget
` (4 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:52 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
`-Wcomma` option of clang was introduced: To identify unintentional uses
of the comma operator.
Intentional uses include situations where one wants to avoid curly
brackets around multiple statements that need to be guarded by a
condition. This is the case here, as the repetitive nature of the
statements is easier to see for a human reader this way. At least in my
opinion.
However, opinions on this differ wildly, take 10 people and you have 10
different preferences.
On the Git mailing list, it seems that the consensus is to use the long
form instead, so let's do just that.
Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
diff-delta.c | 38 +++++++++++++++++++++++++-------------
1 file changed, 25 insertions(+), 13 deletions(-)
diff --git a/diff-delta.c b/diff-delta.c
index a4faf73829b..71d37368d68 100644
--- a/diff-delta.c
+++ b/diff-delta.c
@@ -438,19 +438,31 @@ create_delta(const struct delta_index *index,
op = out + outpos++;
i = 0x80;
- if (moff & 0x000000ff)
- out[outpos++] = moff >> 0, i |= 0x01;
- if (moff & 0x0000ff00)
- out[outpos++] = moff >> 8, i |= 0x02;
- if (moff & 0x00ff0000)
- out[outpos++] = moff >> 16, i |= 0x04;
- if (moff & 0xff000000)
- out[outpos++] = moff >> 24, i |= 0x08;
-
- if (msize & 0x00ff)
- out[outpos++] = msize >> 0, i |= 0x10;
- if (msize & 0xff00)
- out[outpos++] = msize >> 8, i |= 0x20;
+ if (moff & 0x000000ff) {
+ out[outpos++] = moff >> 0;
+ i |= 0x01;
+ }
+ if (moff & 0x0000ff00) {
+ out[outpos++] = moff >> 8;
+ i |= 0x02;
+ }
+ if (moff & 0x00ff0000) {
+ out[outpos++] = moff >> 16;
+ i |= 0x04;
+ }
+ if (moff & 0xff000000) {
+ out[outpos++] = moff >> 24;
+ i |= 0x08;
+ }
+
+ if (msize & 0x00ff) {
+ out[outpos++] = msize >> 0;
+ i |= 0x10;
+ }
+ if (msize & 0xff00) {
+ out[outpos++] = msize >> 8;
+ i |= 0x20;
+ }
*op = i;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 07/10] wildmatch: avoid using of the comma operator
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (5 preceding siblings ...)
2025-03-27 11:52 ` [PATCH v3 06/10] diff-delta: avoid using the comma operator Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:53 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:53 ` [PATCH v3 08/10] compat/regex: explicitly mark intentional use " Johannes Schindelin via GitGitGadget
` (3 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:53 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
`-Wcomma` option of clang was introduced: To identify unintentional uses
of the comma operator.
In this instance, the usage is intentional because it allows storing the
value of the current character as `prev_ch` before making the next
character the current one, all of which happens in the loop condition
that lets the loop stop at a closing bracket.
However, it is hard to read.
The chosen alternative to using the comma operator is to move those
assignments from the condition into the loop body; In this particular
case that requires special care because the loop body contains a
`continue` for the case where a character class is found that starts
with `[:` but does not end in `:]` (and the assignments should occur
even when that code path is taken), which needs to be turned into a
`goto`.
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
wildmatch.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/wildmatch.c b/wildmatch.c
index 8ea29141bd7..69a2ae7000d 100644
--- a/wildmatch.c
+++ b/wildmatch.c
@@ -223,7 +223,7 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
p_ch = '[';
if (t_ch == p_ch)
matched = 1;
- continue;
+ goto next;
}
if (CC_EQ(s,i, "alnum")) {
if (ISALNUM(t_ch))
@@ -268,7 +268,10 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
p_ch = 0; /* This makes "prev_ch" get set to 0. */
} else if (t_ch == p_ch)
matched = 1;
- } while (prev_ch = p_ch, (p_ch = *++p) != ']');
+next:
+ prev_ch = p_ch;
+ p_ch = *++p;
+ } while (p_ch != ']');
if (matched == negated ||
((flags & WM_PATHNAME) && t_ch == '/'))
return WM_NOMATCH;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 08/10] compat/regex: explicitly mark intentional use of the comma operator
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (6 preceding siblings ...)
2025-03-27 11:53 ` [PATCH v3 07/10] wildmatch: avoid using of " Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:53 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:53 ` [PATCH v3 09/10] clang: warn when the comma operator is used Johannes Schindelin via GitGitGadget
` (2 subsequent siblings)
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:53 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
The comma operator is a somewhat obscure C feature that is often used by
mistake and can even cause unintentional code flow. That is why the
`-Wcomma` option of clang was introduced: To identify unintentional uses
of the comma operator.
In the `compat/regex/` code, the comma operator is used twice, once to
avoid surrounding two conditional statements with curly brackets, the
other one to increment two counters simultaneously in a `do ... while`
condition.
The first one is replaced with a proper conditional block, surrounded by
curly brackets.
The second one would be harder to replace because the loop contains two
`continue`s. Therefore, the second one is marked as intentional by
casting the value-to-discard to `void`.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
compat/regex/regex_internal.c | 5 ++++-
compat/regex/regexec.c | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
index ec5cc5d2dd1..4a4f849629a 100644
--- a/compat/regex/regex_internal.c
+++ b/compat/regex/regex_internal.c
@@ -1232,7 +1232,10 @@ re_node_set_merge (re_node_set *dest, const re_node_set *src)
is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
{
if (dest->elems[id] == src->elems[is])
- is--, id--;
+ {
+ is--;
+ id--;
+ }
else if (dest->elems[id] < src->elems[is])
dest->elems[--sbase] = src->elems[is--];
else /* if (dest->elems[id] > src->elems[is]) */
diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
index 2eeec82f407..c08f1bbe1f5 100644
--- a/compat/regex/regexec.c
+++ b/compat/regex/regexec.c
@@ -2210,7 +2210,7 @@ sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,
/* mctx->bkref_ents may have changed, reload the pointer. */
entry = mctx->bkref_ents + enabled_idx;
}
- while (enabled_idx++, entry++->more);
+ while ((void)enabled_idx++, entry++->more);
}
err = REG_NOERROR;
free_return:
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 09/10] clang: warn when the comma operator is used
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (7 preceding siblings ...)
2025-03-27 11:53 ` [PATCH v3 08/10] compat/regex: explicitly mark intentional use " Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:53 ` Johannes Schindelin via GitGitGadget
2025-03-27 11:53 ` [PATCH v3 10/10] detect-compiler: detect clang even if it found CUDA Johannes Schindelin via GitGitGadget
2025-03-27 15:07 ` [PATCH v3 00/10] Avoid the comma operator Phillip Wood
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:53 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
When compiling Git using `clang`, the `-Wcomma` option can be used to
warn about code using the comma operator (because it is typically
unintentional and wants to use the semicolon instead).
Helped-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
config.mak.dev | 4 ++++
meson.build | 1 +
2 files changed, 5 insertions(+)
diff --git a/config.mak.dev b/config.mak.dev
index 0fd8cc4d355..31423638169 100644
--- a/config.mak.dev
+++ b/config.mak.dev
@@ -40,6 +40,10 @@ DEVELOPER_CFLAGS += -Wvla
DEVELOPER_CFLAGS += -Wwrite-strings
DEVELOPER_CFLAGS += -fno-common
+ifneq ($(filter clang9,$(COMPILER_FEATURES)),)
+DEVELOPER_CFLAGS += -Wcomma
+endif
+
ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
endif
diff --git a/meson.build b/meson.build
index efe2871c9db..fd8c05dec91 100644
--- a/meson.build
+++ b/meson.build
@@ -715,6 +715,7 @@ libgit_dependencies = [ ]
# Makefile.
if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'
foreach cflag : [
+ '-Wcomma',
'-Wdeclaration-after-statement',
'-Wformat-security',
'-Wold-style-definition',
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* [PATCH v3 10/10] detect-compiler: detect clang even if it found CUDA
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (8 preceding siblings ...)
2025-03-27 11:53 ` [PATCH v3 09/10] clang: warn when the comma operator is used Johannes Schindelin via GitGitGadget
@ 2025-03-27 11:53 ` Johannes Schindelin via GitGitGadget
2025-03-27 15:07 ` [PATCH v3 00/10] Avoid the comma operator Phillip Wood
10 siblings, 0 replies; 64+ messages in thread
From: Johannes Schindelin via GitGitGadget @ 2025-03-27 11:53 UTC (permalink / raw)
To: git
Cc: Philip Oakley, Patrick Steinhardt, Phillip Wood, Karthik Nayak,
Jeff King, Taylor Blau, Eric Sunshine, Chris Torek,
Johannes Schindelin, Johannes Schindelin
From: Johannes Schindelin <johannes.schindelin@gmx.de>
In my setup, clang finds `/usr/local/cuda` and hence the output of
`clang -v` ends with this line:
Found CUDA installation: /usr/local/cuda, version
This confuses the `detect-compiler` script because it matches _all_
lines that contain the needle "version" surrounded by spaces. As a
consequence, the `get_family` function returns two lines: "Ubuntu clang"
and above-mentioned line, which the `case` statement does not handle
well and hence reports "unknown compiler family" instead of the expected
set of "clang14", "clang13", ..., "clang1" output.
Let's unconfuse the script by letting it parse the first matching line
and ignore the rest.
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
detect-compiler | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/detect-compiler b/detect-compiler
index a87650b71bb..124ebdd4c9d 100755
--- a/detect-compiler
+++ b/detect-compiler
@@ -9,7 +9,7 @@ CC="$*"
#
# FreeBSD clang version 3.4.1 (tags/RELEASE...)
get_version_line() {
- LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
+ LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}'
}
get_family() {
--
gitgitgadget
^ permalink raw reply related [flat|nested] 64+ messages in thread* Re: [PATCH v3 00/10] Avoid the comma operator
2025-03-27 11:52 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
` (9 preceding siblings ...)
2025-03-27 11:53 ` [PATCH v3 10/10] detect-compiler: detect clang even if it found CUDA Johannes Schindelin via GitGitGadget
@ 2025-03-27 15:07 ` Phillip Wood
2025-03-29 0:39 ` Junio C Hamano
10 siblings, 1 reply; 64+ messages in thread
From: Phillip Wood @ 2025-03-27 15:07 UTC (permalink / raw)
To: Johannes Schindelin via GitGitGadget, git
Cc: Philip Oakley, Patrick Steinhardt, Karthik Nayak, Jeff King,
Taylor Blau, Eric Sunshine, Chris Torek, Johannes Schindelin
Hi Johannes
On 27/03/2025 11:52, Johannes Schindelin via GitGitGadget wrote:
>
> Changes since v2:
>
> * Made the sed construct in detect-compiler portable (thanks, Eric
> Sunshine!)
> * The majority of the feedback disagreed with the more compact format in
> diff-delta.c, so I changed it to the long format (thanks, Phillip Wood!)
> * The more succinct and safer, but less readable, cast in the loop
> condition of the dowild() function was replaced with the goto-based
> alternative I had mentioned as a possibility in the commit message
> (thanks, Phillip Wood!)
> * I adjusted the style of my compat/regex/ patch to the surrounding code's.
> * The -Wcomma option is now used in Meson-based clang builds, too (thanks,
> Patrick Steinhardt!)
The range-diff below looks good to me, thanks for making our code base
clearer.
Best Wishes
Phillip
> Range-diff vs v2:
>
> 1: 913c7a0d296 = 1: 913c7a0d296 remote-curl: avoid using the comma operator unnecessarily
> 2: 37ff88b8275 = 2: 37ff88b8275 rebase: avoid using the comma operator unnecessarily
> 3: f601f4e74a5 = 3: f601f4e74a5 kwset: avoid using the comma operator unnecessarily
> 4: f60ebe376e1 = 4: f60ebe376e1 clar: avoid using the comma operator unnecessarily
> 5: 7239078413f = 5: 7239078413f xdiff: avoid using the comma operator unnecessarily
> 6: 5e0e8325620 ! 6: 045d695d73e diff-delta: explicitly mark intentional use of the comma operator
> @@ Metadata
> Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> ## Commit message ##
> - diff-delta: explicitly mark intentional use of the comma operator
> + diff-delta: avoid using the comma operator
>
> The comma operator is a somewhat obscure C feature that is often used by
> mistake and can even cause unintentional code flow. That is why the
> @@ Commit message
> Intentional uses include situations where one wants to avoid curly
> brackets around multiple statements that need to be guarded by a
> condition. This is the case here, as the repetitive nature of the
> - statements is easier to see for a human reader this way.
> + statements is easier to see for a human reader this way. At least in my
> + opinion.
>
> - To mark this usage as intentional, the return value of the statement
> - before the comma needs to be cast to `void`, which we do here.
> + However, opinions on this differ wildly, take 10 people and you have 10
> + different preferences.
>
> + On the Git mailing list, it seems that the consensus is to use the long
> + form instead, so let's do just that.
> +
> + Suggested-by: Phillip Wood <phillip.wood123@gmail.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> ## diff-delta.c ##
> @@ diff-delta.c: create_delta(const struct delta_index *index,
> + op = out + outpos++;
> i = 0x80;
>
> - if (moff & 0x000000ff)
> +- if (moff & 0x000000ff)
> - out[outpos++] = moff >> 0, i |= 0x01;
> -+ (void)(out[outpos++] = moff >> 0), i |= 0x01;
> - if (moff & 0x0000ff00)
> +- if (moff & 0x0000ff00)
> - out[outpos++] = moff >> 8, i |= 0x02;
> -+ (void)(out[outpos++] = moff >> 8), i |= 0x02;
> - if (moff & 0x00ff0000)
> +- if (moff & 0x00ff0000)
> - out[outpos++] = moff >> 16, i |= 0x04;
> -+ (void)(out[outpos++] = moff >> 16), i |= 0x04;
> - if (moff & 0xff000000)
> +- if (moff & 0xff000000)
> - out[outpos++] = moff >> 24, i |= 0x08;
> -+ (void)(out[outpos++] = moff >> 24), i |= 0x08;
> -
> - if (msize & 0x00ff)
> +-
> +- if (msize & 0x00ff)
> - out[outpos++] = msize >> 0, i |= 0x10;
> -+ (void)(out[outpos++] = msize >> 0), i |= 0x10;
> - if (msize & 0xff00)
> +- if (msize & 0xff00)
> - out[outpos++] = msize >> 8, i |= 0x20;
> -+ (void)(out[outpos++] = msize >> 8), i |= 0x20;
> ++ if (moff & 0x000000ff) {
> ++ out[outpos++] = moff >> 0;
> ++ i |= 0x01;
> ++ }
> ++ if (moff & 0x0000ff00) {
> ++ out[outpos++] = moff >> 8;
> ++ i |= 0x02;
> ++ }
> ++ if (moff & 0x00ff0000) {
> ++ out[outpos++] = moff >> 16;
> ++ i |= 0x04;
> ++ }
> ++ if (moff & 0xff000000) {
> ++ out[outpos++] = moff >> 24;
> ++ i |= 0x08;
> ++ }
> ++
> ++ if (msize & 0x00ff) {
> ++ out[outpos++] = msize >> 0;
> ++ i |= 0x10;
> ++ }
> ++ if (msize & 0xff00) {
> ++ out[outpos++] = msize >> 8;
> ++ i |= 0x20;
> ++ }
>
> *op = i;
>
> 7: 9a6de12b807 ! 7: 1d0ce59cb68 wildmatch: explicitly mark intentional use of the comma operator
> @@ Metadata
> Author: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>
> ## Commit message ##
> - wildmatch: explicitly mark intentional use of the comma operator
> + wildmatch: avoid using of the comma operator
>
> The comma operator is a somewhat obscure C feature that is often used by
> mistake and can even cause unintentional code flow. That is why the
> `-Wcomma` option of clang was introduced: To identify unintentional uses
> of the comma operator.
>
> - To mark such a usage as intentional, the value needs to be cast to
> - `void`, which we do here.
> -
> In this instance, the usage is intentional because it allows storing the
> value of the current character as `prev_ch` before making the next
> character the current one, all of which happens in the loop condition
> that lets the loop stop at a closing bracket.
>
> - The alternative to using the comma operator would be to move those
> + However, it is hard to read.
> +
> + The chosen alternative to using the comma operator is to move those
> assignments from the condition into the loop body; In this particular
> - case that would require the assignments to either be duplicated or to
> - introduce and use a `goto` target before the assignments, though,
> - because the loop body contains a `continue` for the case where a
> - character class is found that starts with `[:` but does not end in `:]`
> - (and the assignments should occur even when that code path is taken).
> + case that requires special care because the loop body contains a
> + `continue` for the case where a character class is found that starts
> + with `[:` but does not end in `:]` (and the assignments should occur
> + even when that code path is taken), which needs to be turned into a
> + `goto`.
>
> + Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> ## wildmatch.c ##
> +@@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> + p_ch = '[';
> + if (t_ch == p_ch)
> + matched = 1;
> +- continue;
> ++ goto next;
> + }
> + if (CC_EQ(s,i, "alnum")) {
> + if (ISALNUM(t_ch))
> @@ wildmatch.c: static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> p_ch = 0; /* This makes "prev_ch" get set to 0. */
> } else if (t_ch == p_ch)
> matched = 1;
> - } while (prev_ch = p_ch, (p_ch = *++p) != ']');
> -+ } while ((void)(prev_ch = p_ch), (p_ch = *++p) != ']');
> ++next:
> ++ prev_ch = p_ch;
> ++ p_ch = *++p;
> ++ } while (p_ch != ']');
> if (matched == negated ||
> ((flags & WM_PATHNAME) && t_ch == '/'))
> return WM_NOMATCH;
> 8: dc626f36df3 ! 8: b8405f3d237 compat/regex: explicitly mark intentional use of the comma operator
> @@ Commit message
>
> ## compat/regex/regex_internal.c ##
> @@ compat/regex/regex_internal.c: re_node_set_merge (re_node_set *dest, const re_node_set *src)
> - for (sbase = dest->nelem + 2 * src->nelem,
> is = src->nelem - 1, id = dest->nelem - 1; is >= 0 && id >= 0; )
> {
> -- if (dest->elems[id] == src->elems[is])
> + if (dest->elems[id] == src->elems[is])
> - is--, id--;
> -- else if (dest->elems[id] < src->elems[is])
> -+ if (dest->elems[id] == src->elems[is]) {
> -+ is--;
> -+ id--;
> -+ } else if (dest->elems[id] < src->elems[is])
> ++ {
> ++ is--;
> ++ id--;
> ++ }
> + else if (dest->elems[id] < src->elems[is])
> dest->elems[--sbase] = src->elems[is--];
> else /* if (dest->elems[id] > src->elems[is]) */
> - --id;
>
> ## compat/regex/regexec.c ##
> @@ compat/regex/regexec.c: sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,
> 9: 91f86c3aba9 ! 9: 6b6cd556465 clang: warn when the comma operator is used
> @@ Commit message
> warn about code using the comma operator (because it is typically
> unintentional and wants to use the semicolon instead).
>
> + Helped-by: Patrick Steinhardt <ps@pks.im>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> ## config.mak.dev ##
> @@ config.mak.dev: DEVELOPER_CFLAGS += -Wvla
> ifneq ($(filter clang4,$(COMPILER_FEATURES)),)
> DEVELOPER_CFLAGS += -Wtautological-constant-out-of-range-compare
> endif
> +
> + ## meson.build ##
> +@@ meson.build: libgit_dependencies = [ ]
> + # Makefile.
> + if get_option('warning_level') in ['2','3', 'everything'] and compiler.get_argument_syntax() == 'gcc'
> + foreach cflag : [
> ++ '-Wcomma',
> + '-Wdeclaration-after-statement',
> + '-Wformat-security',
> + '-Wold-style-definition',
> 10: 2f6f31240fe ! 10: 77f1dcaca1c detect-compiler: detect clang even if it found CUDA
> @@ Commit message
> Let's unconfuse the script by letting it parse the first matching line
> and ignore the rest.
>
> + Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> ## detect-compiler ##
> @@ detect-compiler: CC="$*"
> # FreeBSD clang version 3.4.1 (tags/RELEASE...)
> get_version_line() {
> - LANG=C LC_ALL=C $CC -v 2>&1 | grep ' version '
> -+ LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q}'
> ++ LANG=C LC_ALL=C $CC -v 2>&1 | sed -n '/ version /{p;q;}'
> }
>
> get_family() {
>
^ permalink raw reply [flat|nested] 64+ messages in thread* Re: [PATCH v3 00/10] Avoid the comma operator
2025-03-27 15:07 ` [PATCH v3 00/10] Avoid the comma operator Phillip Wood
@ 2025-03-29 0:39 ` Junio C Hamano
0 siblings, 0 replies; 64+ messages in thread
From: Junio C Hamano @ 2025-03-29 0:39 UTC (permalink / raw)
To: Phillip Wood
Cc: Johannes Schindelin via GitGitGadget, git, Philip Oakley,
Patrick Steinhardt, Karthik Nayak, Jeff King, Taylor Blau,
Eric Sunshine, Chris Torek, Johannes Schindelin
Phillip Wood <phillip.wood123@gmail.com> writes:
> Hi Johannes
>
> On 27/03/2025 11:52, Johannes Schindelin via GitGitGadget wrote:
>> Changes since v2:
>> * Made the sed construct in detect-compiler portable (thanks, Eric
>> Sunshine!)
>> * The majority of the feedback disagreed with the more compact format in
>> diff-delta.c, so I changed it to the long format (thanks, Phillip Wood!)
>> * The more succinct and safer, but less readable, cast in the loop
>> condition of the dowild() function was replaced with the goto-based
>> alternative I had mentioned as a possibility in the commit message
>> (thanks, Phillip Wood!)
>> * I adjusted the style of my compat/regex/ patch to the surrounding code's.
>> * The -Wcomma option is now used in Meson-based clang builds, too (thanks,
>> Patrick Steinhardt!)
>
> The range-diff below looks good to me, thanks for making our code base
> clearer.
>
> Best Wishes
>
> Phillip
Yup, thanks Dscho, and all who gave valuable input to polish the
series.
Will queue.
^ permalink raw reply [flat|nested] 64+ messages in thread