From 913c7a0d29699c2bd80fefcdc00879c400f1e7db Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 14 Dec 2022 21:31:40 +0100 Subject: [PATCH 01/10] remote-curl: avoid using the comma operator unnecessarily 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 --- remote-curl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/remote-curl.c b/remote-curl.c index 1273507a96cae9..590b228f67fcbc 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) From 37ff88b8275cf4d6b0c715a99f4572e70d6e3729 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Wed, 14 Dec 2022 21:33:30 +0100 Subject: [PATCH 02/10] rebase: avoid using the comma operator unnecessarily 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 --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index d4715ed35d77ed..62bdf7276f71a8 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; From f601f4e74a5e8146f2926a725f3d1522b928eb51 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 21:59:04 +0100 Subject: [PATCH 03/10] kwset: avoid using the comma operator unnecessarily 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 --- kwset.c | 54 +++++++++++++++++++++++++++++------------------------- 1 file changed, 29 insertions(+), 25 deletions(-) diff --git a/kwset.c b/kwset.c index 1714eada608b86..064329434e56d1 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; } From f60ebe376e10d7741f6bd657874a17f6c09d4477 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 22:00:14 +0100 Subject: [PATCH 04/10] clar: avoid using the comma operator unnecessarily 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 --- 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 8b206179fc4ea1..2203743fb48046 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; From 7239078413f6a223105939af1b56e79b9d302f1f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 22:01:06 +0100 Subject: [PATCH 05/10] xdiff: avoid using the comma operator unnecessarily 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 --- xdiff/xdiffi.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c index 8889b8b62a1a6e..5a96e36dfbeab1 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; From 045d695d73e059ac9d56570c9a2e1176b9c0c1d7 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 21:49:46 +0100 Subject: [PATCH 06/10] 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 `-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 Signed-off-by: Johannes Schindelin --- diff-delta.c | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/diff-delta.c b/diff-delta.c index a4faf73829be00..71d37368d68a18 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; From 1d0ce59cb684c2878f1a0db9daeae23ef4abb763 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 21:52:44 +0100 Subject: [PATCH 07/10] 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. 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 Signed-off-by: Johannes Schindelin --- wildmatch.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/wildmatch.c b/wildmatch.c index 8ea29141bd7c52..69a2ae7000d512 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; From b8405f3d23734b860bd222705d498bde63136432 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 21:52:44 +0100 Subject: [PATCH 08/10] compat/regex: explicitly mark intentional use 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. 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 --- 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 ec5cc5d2dd10f8..4a4f849629a26e 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 2eeec82f4077b7..c08f1bbe1f5ecf 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: From 6b6cd556465f21e43536706c88c49f8790a2dc5f Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 22:03:52 +0100 Subject: [PATCH 09/10] clang: warn when the comma operator is used 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 Signed-off-by: Johannes Schindelin --- config.mak.dev | 4 ++++ meson.build | 1 + 2 files changed, 5 insertions(+) diff --git a/config.mak.dev b/config.mak.dev index 0fd8cc4d355ebb..31423638169a67 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 efe2871c9dba13..fd8c05dec91497 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', From 77f1dcaca1c9df9e24880680311e5bb3eaeec1f8 Mon Sep 17 00:00:00 2001 From: Johannes Schindelin Date: Tue, 25 Mar 2025 22:06:05 +0100 Subject: [PATCH 10/10] detect-compiler: detect clang even if it found CUDA 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 Signed-off-by: Johannes Schindelin --- detect-compiler | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/detect-compiler b/detect-compiler index a87650b71bb095..124ebdd4c9d1b3 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() {