Avoid the comma operator

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.

Johannes Schindelin (2):
remote-curl: avoid using the comma operator unnecessarily
rebase: avoid using the comma operator unnecessarily

 builtin/rebase.c | 2 +-
 remote-curl.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1889

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 remote-curl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1273507a96c..57b515b37e7 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d4715ed35d7..62bdf7276f7 100644
@@ -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

Minor aside: How were these 'discovered'?

Philip

The changes look obviously good to me, thanks. The reftable library and backend also had several instances where the operator was used by accident, and I've gotten rid of those over time. They typically don't do any harm as the result is essentially the same, but sometimes they may cause issues. And at the very least they cause confusion.

It would be great if there was a compiler warning we could enable for cases where the operator likely isn't intentional. But I couldn't find any, unfortunately.

Thanks!

Patrick

Hi Philip,

I am working on a GitHub workflow that uses CodeQL to find such issues, that's how I found them. (I also worked with the CodeQL team to get this query added, way back when I was still working at GitHub.)

Ciao,
Johannes

Hi Patrick,

Thank you for addressing these in the reftable library!

I was not actually planning on adding the CodeQL workflow to git/git, seeing as its CI is already taking way too much CPU time for my liking. But in microsoft/git, I am kind of required to, so we'll catch those issues there.

Ciao,
Johannes

Clang does have '-Wcomma' [1], which seems to be exactly what we want.

[1]: https://clang.llvm.org/docs/DiagnosticsReference.html#wcomma

Hi Johannes

clang's -Wcomma finds another instance in this file

@@ -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);

In fact it finds a surprising number in our code base. I was worried there would be a lot of false positives but so far all of the ones I've looked at would be better not using a ","

Best Wishes

Phillip

It looks like there are a few tricky cases. Inside a loop condition, a comma can be used to smuggle in an extra line. E.g., in wildmatch:

  do {
    ...
  } while (prev_ch = p_ch, (p_ch = *++p) != ']');

or there are a few spots in clar like:

  while ((d = (errno = 0, readdir(source_dir))) != NULL) {
    ...
  }
  cl_assert_(errno == 0, "Failed to iterate source dir");

These could probably be re-written, but it's not a totally trivial change.

The rest of them seem pretty straight-forward (though you do have to watch out for conditionals using it to stuff multiple lines into a single "statement"):

index d4715ed35d..62bdf7276f 100644
@@ -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;

index a4faf73829..71d37368d6 100644
@@ -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;
 

index 1273507a96..590b228f67 100644
@@ -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)

index 8889b8b62a..5a96e36dfb 100644
@@ -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;

-Peff

"Johannes Schindelin via GitGitGadget" <[email protected]>

Well spotted.

These two looked somehow surprisingly bad.

If I hadn't known better, I may have spent quite some time wondering if these are some ways to hide an unexpected behaviour behind the differences between a comma and a semicolon for nefarious purposes.

Will queue. Thanks.

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).

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: explicitly mark intentional use of the comma operator
wildmatch: explicitly mark intentional use 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 |  7 +++--
 compat/regex/regexec.c        |  2 +-
 config.mak.dev                |  4 +++
 detect-compiler               |  2 +-
 diff-delta.c                  | 12 ++++----
 kwset.c                       | 54 +++++++++++++++++++----------------
 remote-curl.c                 |  4 +--
 t/unit-tests/clar/clar/fs.h   | 10 +++++--
 wildmatch.c                   |  2 +-
 xdiff/xdiffi.c                | 12 +++++---
 11 files changed, 65 insertions(+), 46 deletions(-)

base-commit: 683c54c999c301c2cd6f715c411407c413b1d84e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1889%2Fdscho%2Fcomma-operator-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1889/dscho/comma-operator-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1889

Range-diff vs v1:

1: e3069fd4564 ! 1: 913c7a0d296 remote-curl: avoid using the comma operator unnecessarily
@@ Commit message

## remote-curl.c ##
+@@ remote-curl.c: 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);
@@ remote-curl.c: static int push_git(struct discovery *heads, int nr_spec, const char **specs)
packet_buf_flush(&preamble);

  2:  7dfbdc48954 =  2:  37ff88b8275 rebase: avoid using the comma operator unnecessarily
  -:  ----------- >  3:  f601f4e74a5 kwset: avoid using the comma operator unnecessarily
  -:  ----------- >  4:  f60ebe376e1 clar: avoid using the comma operator unnecessarily
  -:  ----------- >  5:  7239078413f xdiff: avoid using the comma operator unnecessarily
  -:  ----------- >  6:  5e0e8325620 diff-delta: explicitly mark intentional use of the comma operator
  -:  ----------- >  7:  9a6de12b807 wildmatch: explicitly mark intentional use of the comma operator
  -:  ----------- >  8:  dc626f36df3 compat/regex: explicitly mark intentional use of the comma operator
  -:  ----------- >  9:  91f86c3aba9 clang: warn when the comma operator is used
  -:  ----------- > 10:  2f6f31240fe detect-compiler: detect clang even if it found CUDA

gitgitgadget

From: Johannes Schindelin <[email protected]>

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.

---
 remote-curl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 1273507a96c..590b228f67f 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d4715ed35d7..62bdf7276f7 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 kwset.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

index 1714eada608..064329434e5 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 t/unit-tests/clar/clar/fs.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 8b206179fc4..2203743fb48 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 xdiff/xdiffi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index 8889b8b62a1..5a96e36dfbe 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 diff-delta.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

index a4faf73829b..a03ba10b2be 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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).

---
 wildmatch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8ea29141bd7..ce8108a6d57 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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 continues. Therefore, the second one is marked as intentional by casting the value-to-discard to void.

---
 compat/regex/regex_internal.c | 7 ++++---
 compat/regex/regexec.c        | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

index ec5cc5d2dd1..7672583bf7e 100644
@@ -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;

index 2eeec82f407..c08f1bbe1f5 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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).

---
 config.mak.dev | 4 ++++
 1 file changed, 4 insertions(+)

index 0fd8cc4d355..31423638169 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 detect-compiler | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a87650b71bb..01eca3a781d 100755
@@ -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

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

Thanks for all of these fixes!

Patrick

Hi Patrick,

Thank you for putting so much energy into improving the tests, I really appreciate it!

Ciao,
Johannes

Hi Patrick,

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

Hi Patrick,

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

"Johannes Schindelin via GitGitGadget" <[email protected]>

 diff-delta.c | 37 ++++++++++++++++++++++++-------------
 wildmatch.c  |  4 +++-
 2 files changed, 27 insertions(+), 14 deletions(-)

index a4faf73829..a999b875d4 100644
@@ -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;
 

index 8ea29141bd..29f4b4df75 100644
@@ -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;

Hi Patrick,

Okay, will do.

For my curiosity...

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

Hi Johannes

I think that would be clearer, something like the diff below

Best Wishes

Phillip

---- >8 ----

index 8ea29141bd7..7230544c356 100644
@@ -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 <[email protected]>

Hi Johannes

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,

Neat. I will be curious to see how the results compare/contrast to what those of us who run Coverity get.

Thanks,
Taylor

Makes sense. I wondered if this:

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

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 should have read this email from you before replying to the last one ;-).

I would actually be interested in having CodeQL support "ship" for copies of git.git that are hosted on GitHub. I wonder if we could do a similar trick as in a56b6230d0 (ci: add a GitHub workflow to submit Coverity scans, 2023-09-25), where the ENABLE_COVERITY_SCAN_FOR_BRANCHES variable controls whether or not the job actually executes.

Thanks,
Taylor

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

(Just picking the latest message to reply to, not really quite right)

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

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

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

Hi Patrick,

Thank you for this valuable reference!

Ciao,
Johannes

Hi Taylor,

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

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 <[email protected]>
      
       ## 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 <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## 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 <[email protected]>

## 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 <[email protected]>

## 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 <[email protected]>

## 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 <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## 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

From: Johannes Schindelin <[email protected]>

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.

---
 remote-curl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

index 1273507a96c..590b228f67f 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index d4715ed35d7..62bdf7276f7 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 kwset.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

index 1714eada608..064329434e5 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 t/unit-tests/clar/clar/fs.h | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

index 8b206179fc4..2203743fb48 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 xdiff/xdiffi.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

index 8889b8b62a1..5a96e36dfbe 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 diff-delta.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

index a4faf73829b..71d37368d68 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 wildmatch.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 8ea29141bd7..69a2ae7000d 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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 continues. Therefore, the second one is marked as intentional by casting the value-to-discard to void.

---
 compat/regex/regex_internal.c | 5 ++++-
 compat/regex/regexec.c        | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

index ec5cc5d2dd1..4a4f849629a 100644
@@ -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]) */

index 2eeec82f407..c08f1bbe1f5 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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).

---
 config.mak.dev | 4 ++++
 meson.build    | 1 +
 2 files changed, 5 insertions(+)

index 0fd8cc4d355..31423638169 100644
@@ -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

index efe2871c9db..fd8c05dec91 100644
@@ -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

From: Johannes Schindelin <[email protected]>

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.

---
 detect-compiler | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index a87650b71bb..124ebdd4c9d 100755
@@ -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

Hi Johannes

The range-diff below looks good to me, thanks for making our code base clearer.

Best Wishes

Phillip

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

Yup, thanks Dscho, and all who gave valuable input to polish the series.

Will queue.