Skip to content

Avoid the comma operator #1889

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 10 commits into from
Closed

Avoid the comma operator #1889

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
913c7a0
remote-curl: avoid using the comma operator unnecessarily
dscho Dec 14, 2022
37ff88b
rebase: avoid using the comma operator unnecessarily
dscho Dec 14, 2022
f601f4e
kwset: avoid using the comma operator unnecessarily
dscho Mar 25, 2025
f60ebe3
clar: avoid using the comma operator unnecessarily
dscho Mar 25, 2025
7239078
xdiff: avoid using the comma operator unnecessarily
dscho Mar 25, 2025
045d695
diff-delta: avoid using the comma operator
dscho Mar 25, 2025
1d0ce59
wildmatch: avoid using of the comma operator
dscho Mar 25, 2025
b8405f3
compat/regex: explicitly mark intentional use of the comma operator
dscho Mar 25, 2025
6b6cd55
clang: warn when the comma operator is used
dscho Mar 25, 2025
77f1dca
detect-compiler: detect clang even if it found CUDA
dscho Mar 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion builtin/rebase.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,
strbuf_addf(&msg, "%s (start): checkout %s",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[email protected]>
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> index d4715ed35d7..62bdf7276f7 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1843,7 +1843,7 @@ int cmd_rebase(int argc,
>   	strbuf_addf(&msg, "%s (start): checkout %s",
>   		    options.reflog_action, options.onto_name);
>   	ropts.oid = &options.onto->object.oid;
> -	ropts.orig_head = &options.orig_head->object.oid,
> +	ropts.orig_head = &options.orig_head->object.oid;

This is obviously a typo - thanks for fixing it

Best Wishes

Phillip

>   	ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD |
>   			RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
>   	ropts.head_msg = msg.buf;

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;
Expand Down
54 changes: 29 additions & 25 deletions kwset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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;
}

Expand Down
4 changes: 2 additions & 2 deletions remote-curl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -1401,7 +1401,7 @@ static int push_git(struct discovery *heads, int nr_spec, const char **specs)
packet_buf_flush(&preamble);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Phillip Wood wrote (reply to this):

Hi Johannes

On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:
> 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.

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

> Signed-off-by: Johannes Schindelin <[email protected]>
> ---
>   remote-curl.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> > diff --git a/remote-curl.c b/remote-curl.c
> index 1273507a96c..57b515b37e7 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -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)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Jeff King wrote (reply to this):

On Tue, Mar 25, 2025 at 04:28:32PM +0000, Phillip Wood wrote:

> On 25/03/2025 08:01, Johannes Schindelin via GitGitGadget wrote:
> > 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.
> 
> 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 ","

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"):

diff --git a/builtin/rebase.c b/builtin/rebase.c
index d4715ed35d..62bdf7276f 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;
diff --git a/diff-delta.c b/diff-delta.c
index a4faf73829..71d37368d6 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;
 
diff --git a/kwset.c b/kwset.c
index 1714eada60..23ab015448 100644
--- a/kwset.c
+++ b/kwset.c
@@ -197,10 +197,14 @@ 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 +261,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 +281,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 +571,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 +653,7 @@ 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;
     }
 
diff --git a/remote-curl.c b/remote-curl.c
index 1273507a96..590b228f67 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)
diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c
index 8889b8b62a..5a96e36dfb 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;

-Peff


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)
Expand Down
10 changes: 8 additions & 2 deletions t/unit-tests/clar/clar/fs.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,9 +376,12 @@ fs_copydir_helper(const char *source, const char *dest, int dest_mode)
mkdir(dest, dest_mode);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Tue, Mar 25, 2025 at 11:32:08PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the Git mailing list, Johannes Schindelin wrote (reply to this):

Hi Patrick,

On Wed, 26 Mar 2025, Patrick Steinhardt wrote:

> On Tue, Mar 25, 2025 at 11:32:08PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <[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.
>
> This code has changed upstream already, but let's roll with your change
> anyway. I plan to update the clar to the upstream version soonish once I
> have landed integer comparisons, and will take care that there aren't
> any other comment operators left.

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

Ciao,
Johannes


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;

Expand Down Expand Up @@ -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;

Expand Down