threads / patch / 64499

patchmingw: avoid the comma operator

Subject: [PATCH] mingw: avoid the comma operator

## tl;dr

3 messages between Nov 17, 2025 and Nov 18, 2025. Diffs are folded; open one to read it.

replies: 2people: 3as markdown or json

Johannes Schindelin via GitGitGadget· Nov 17, 2025, 20:46 UTC · lore
From: Johannes Schindelin <johannes.schindelin@gmx.de>

The pattern `return errno = ..., -1;` is observed several times in `compat/mingw.c`. It has served us well over the years, but now clang starts complaining:

  compat/mingw.c:723:24: error: possible misuse of comma operator here [-Werror,-Wcomma]
    723 |                 return errno = ENOSYS, -1;
        |                                      ^

See for example this failing workflow run: https://github.com/git-for-windows/git-sdk-arm64/actions/runs/15457893907/job/43513458823#step:8:201

Let's appease clang (and also reduce the use of the no longer common comma operator).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
    mingw: avoid the comma operator
    
    I wonder how many more times I will deal with the comma operator...
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2007%2Fdscho%2Fmingw-avoid-the-comma-operator-5660--v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2007/dscho/mingw-avoid-the-comma-operator-5660--v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2007
 compat/mingw.c | 48 ++++++++++++++++++++++++++++--------------------
 1 file changed, 28 insertions(+), 20 deletions(-)
Show changes to compat/mingw.c +28 −20
diff --git a/compat/mingw.c b/compat/mingw.c
index 736a07a028..90ba5cea9d 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -491,8 +491,10 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
 	DWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;
 
 	/* only these flags are supported */
-	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
-		return errno = ENOSYS, -1;
+	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {
+		errno = ENOSYS;
+		return -1;
+	}
 
 	/*
 	 * FILE_SHARE_WRITE is required to permit child processes
@@ -2450,12 +2452,14 @@ static int start_timer_thread(void)
 	timer_event = CreateEvent(NULL, FALSE, FALSE, NULL);
 	if (timer_event) {
 		timer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);
-		if (!timer_thread )
-			return errno = ENOMEM,
-				error("cannot start timer thread");
-	} else
-		return errno = ENOMEM,
-			error("cannot allocate resources for timer");
+		if (!timer_thread ) {
+			errno = ENOMEM;
+			return error("cannot start timer thread");
+		}
+	} else {
+		errno = ENOMEM;
+		return error("cannot allocate resources for timer");
+	}
 	return 0;
 }
 
@@ -2488,13 +2492,15 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)
 	static const struct timeval zero;
 	static int atexit_done;
 
-	if (out)
-		return errno = EINVAL,
-			error("setitimer param 3 != NULL not implemented");
+	if (out) {
+		errno = EINVAL;
+		return error("setitimer param 3 != NULL not implemented");
+	}
 	if (!is_timeval_eq(&in->it_interval, &zero) &&
-	    !is_timeval_eq(&in->it_interval, &in->it_value))
-		return errno = EINVAL,
-			error("setitimer: it_interval must be zero or eq it_value");
+	    !is_timeval_eq(&in->it_interval, &in->it_value)) {
+		errno = EINVAL;
+		return error("setitimer: it_interval must be zero or eq it_value");
+	}
 
 	if (timer_thread)
 		stop_timer_thread();
@@ -2516,12 +2522,14 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out)
 {
 	if (sig == SIGCHLD)
 		return -1;
-	else if (sig != SIGALRM)
-		return errno = EINVAL,
-			error("sigaction only implemented for SIGALRM");
-	if (out)
-		return errno = EINVAL,
-			error("sigaction: param 3 != NULL not implemented");
+	else if (sig != SIGALRM) {
+		errno = EINVAL;
+		return error("sigaction only implemented for SIGALRM");
+	}
+	if (out) {
+		errno = EINVAL;
+		return error("sigaction: param 3 != NULL not implemented");
+	}
 
 	timer_fn = in->sa_handler;
 	return 0;

base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed

-- 
gitgitgadget
Junio C Hamano· Nov 17, 2025, 22:16 UTC · re: Johannes Schindelin via GitGitGadget · lore

Re: [PATCH] mingw: avoid the comma operator

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 21 quoted lines
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The pattern `return errno = ..., -1;` is observed several times in
> `compat/mingw.c`. It has served us well over the years, but now clang
> starts complaining:
>
>   compat/mingw.c:723:24: error: possible misuse of comma operator here [-Werror,-Wcomma]
>     723 |                 return errno = ENOSYS, -1;
>         |                                      ^
>
> See for example this failing workflow run:
> https://github.com/git-for-windows/git-sdk-arm64/actions/runs/15457893907/job/43513458823#step:8:201
>
> Let's appease clang (and also reduce the use of the no longer common
> comma operator).
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     mingw: avoid the comma operator
>     
>     I wonder how many more times I will deal with the comma operator...
;-)
Show 7 quoted lines
>  	/* only these flags are supported */
> -	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
> -		return errno = ENOSYS, -1;
> +	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {
> +		errno = ENOSYS;
> +		return -1;
> +	}

Good riddance. It indeed is somewhat hard to read, especially because it may not be apparent to readers how "A = B, C" binds (answer: B gets assigned to A and then the whole thing yields C).

I wonder if
	return (errno = ENOSYS), -1;

is accepted by the compiler, but in these error handling we do not have to be cute, and updated code that is both simple and stupid reads very well.

Will queue.  Thanks.
Jeff King· Nov 18, 2025, 09:49 UTC · re: Junio C Hamano · lore

Re: [PATCH] mingw: avoid the comma operator

On Mon, Nov 17, 2025 at 02:16:34PM -0800, Junio C Hamano wrote:
Show 19 quoted lines
> >  	/* only these flags are supported */
> > -	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
> > -		return errno = ENOSYS, -1;
> > +	if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {
> > +		errno = ENOSYS;
> > +		return -1;
> > +	}
> 
> Good riddance.  It indeed is somewhat hard to read, especially
> because it may not be apparent to readers how "A = B, C" binds
> (answer: B gets assigned to A and then the whole thing yields C).
> 
> I wonder if
> 
> 	return (errno = ENOSYS), -1;
> 
> is accepted by the compiler, but in these error handling we do not
> have to be cute, and updated code that is both simple and stupid
> reads very well.

Agreed that we are best avoiding comma operators when we can. There is one spot where we use it, though, and I haven't figured out a good way around it: in the error() macro wrapper.

I guess it does not cause the same compiler complaints because there is no assignment in it. So if nobody is complaining, we can just avert our eyes when looking at the macro. :)

-Peff

← back to recent threads