[PATCH v3 00/10] Avoid the comma operator
- From
Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com>
- Date
- Mar 27, 2025, 11:52 UTC
- Message-ID
- <pull.1889.v3.git.1743076383.gitgitgadget@gmail.com>
- In-Reply-To
- <pull.1889.v2.git.1742945534.gitgitgadget@gmail.com>
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