{"thread":{"id":"64499","subject":"[PATCH] mingw: avoid the comma operator","startedAt":"2025-11-17T20:46:17Z","lastAt":"2025-11-18T09:49:19Z","messageCount":3,"participants":["Johannes Schindelin via GitGitGadget","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530838","messageId":"pull.2007.git.1763412374866.gitgitgadget@gmail.com","threadId":"64499","inReplyTo":null,"subject":"[PATCH] mingw: avoid the comma operator","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2025-11-17T20:46:14Z","receivedAt":"2025-11-17T20:46:17Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThe pattern `return errno = ..., -1;` is observed several times in\n`compat/mingw.c`. It has served us well over the years, but now clang\nstarts complaining:\n\n compat/mingw.c:723:24: error: possible misuse of comma operator here [-Werror,-Wcomma]\n 723 | return errno = ENOSYS, -1;\n | ^\n\nSee for example this failing workflow run:\nhttps://github.com/git-for-windows/git-sdk-arm64/actions/runs/15457893907/job/43513458823#step:8:201\n\nLet's appease clang (and also reduce the use of the no longer common\ncomma operator).\n\nSigned-off-by: Johannes Schindelin \n---\n mingw: avoid the comma operator\n \n I wonder how many more times I will deal with the comma operator...\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2007%2Fdscho%2Fmingw-avoid-the-comma-operator-5660--v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2007/dscho/mingw-avoid-the-comma-operator-5660--v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2007\n\n compat/mingw.c | 48 ++++++++++++++++++++++++++++--------------------\n 1 file changed, 28 insertions(+), 20 deletions(-)\n\ndiff --git a/compat/mingw.c b/compat/mingw.c\nindex 736a07a028..90ba5cea9d 100644\n--- a/compat/mingw.c\n+++ b/compat/mingw.c\n@@ -491,8 +491,10 @@ static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)\n \tDWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;\n \n \t/* only these flags are supported */\n-\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))\n-\t\treturn errno = ENOSYS, -1;\n+\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {\n+\t\terrno = ENOSYS;\n+\t\treturn -1;\n+\t}\n \n \t/*\n \t * FILE_SHARE_WRITE is required to permit child processes\n@@ -2450,12 +2452,14 @@ static int start_timer_thread(void)\n \ttimer_event = CreateEvent(NULL, FALSE, FALSE, NULL);\n \tif (timer_event) {\n \t\ttimer_thread = (HANDLE) _beginthreadex(NULL, 0, ticktack, NULL, 0, NULL);\n-\t\tif (!timer_thread )\n-\t\t\treturn errno = ENOMEM,\n-\t\t\t\terror(\"cannot start timer thread\");\n-\t} else\n-\t\treturn errno = ENOMEM,\n-\t\t\terror(\"cannot allocate resources for timer\");\n+\t\tif (!timer_thread ) {\n+\t\t\terrno = ENOMEM;\n+\t\t\treturn error(\"cannot start timer thread\");\n+\t\t}\n+\t} else {\n+\t\terrno = ENOMEM;\n+\t\treturn error(\"cannot allocate resources for timer\");\n+\t}\n \treturn 0;\n }\n \n@@ -2488,13 +2492,15 @@ int setitimer(int type UNUSED, struct itimerval *in, struct itimerval *out)\n \tstatic const struct timeval zero;\n \tstatic int atexit_done;\n \n-\tif (out)\n-\t\treturn errno = EINVAL,\n-\t\t\terror(\"setitimer param 3 != NULL not implemented\");\n+\tif (out) {\n+\t\terrno = EINVAL;\n+\t\treturn error(\"setitimer param 3 != NULL not implemented\");\n+\t}\n \tif (!is_timeval_eq(&in->it_interval, &zero) &&\n-\t !is_timeval_eq(&in->it_interval, &in->it_value))\n-\t\treturn errno = EINVAL,\n-\t\t\terror(\"setitimer: it_interval must be zero or eq it_value\");\n+\t !is_timeval_eq(&in->it_interval, &in->it_value)) {\n+\t\terrno = EINVAL;\n+\t\treturn error(\"setitimer: it_interval must be zero or eq it_value\");\n+\t}\n \n \tif (timer_thread)\n \t\tstop_timer_thread();\n@@ -2516,12 +2522,14 @@ int sigaction(int sig, struct sigaction *in, struct sigaction *out)\n {\n \tif (sig == SIGCHLD)\n \t\treturn -1;\n-\telse if (sig != SIGALRM)\n-\t\treturn errno = EINVAL,\n-\t\t\terror(\"sigaction only implemented for SIGALRM\");\n-\tif (out)\n-\t\treturn errno = EINVAL,\n-\t\t\terror(\"sigaction: param 3 != NULL not implemented\");\n+\telse if (sig != SIGALRM) {\n+\t\terrno = EINVAL;\n+\t\treturn error(\"sigaction only implemented for SIGALRM\");\n+\t}\n+\tif (out) {\n+\t\terrno = EINVAL;\n+\t\treturn error(\"sigaction: param 3 != NULL not implemented\");\n+\t}\n \n \ttimer_fn = in->sa_handler;\n \treturn 0;\n\nbase-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed\n-- \ngitgitgadget\n"},{"id":"530859","messageId":"xmqqy0o4gv99.fsf@gitster.g","threadId":"64499","inReplyTo":"pull.2007.git.1763412374866.gitgitgadget@gmail.com","subject":"Re: [PATCH] mingw: avoid the comma operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-17T22:16:34Z","receivedAt":"2025-11-17T22:16:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> From: Johannes Schindelin \n>\n> The pattern `return errno = ..., -1;` is observed several times in\n> `compat/mingw.c`. It has served us well over the years, but now clang\n> starts complaining:\n>\n> compat/mingw.c:723:24: error: possible misuse of comma operator here [-Werror,-Wcomma]\n> 723 | return errno = ENOSYS, -1;\n> | ^\n>\n> See for example this failing workflow run:\n> https://github.com/git-for-windows/git-sdk-arm64/actions/runs/15457893907/job/43513458823#step:8:201\n>\n> Let's appease clang (and also reduce the use of the no longer common\n> comma operator).\n>\n> Signed-off-by: Johannes Schindelin \n> ---\n> mingw: avoid the comma operator\n> \n> I wonder how many more times I will deal with the comma operator...\n\n;-)\n\n> \t/* only these flags are supported */\n> -\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))\n> -\t\treturn errno = ENOSYS, -1;\n> +\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {\n> +\t\terrno = ENOSYS;\n> +\t\treturn -1;\n> +\t}\n\nGood riddance. It indeed is somewhat hard to read, especially\nbecause it may not be apparent to readers how \"A = B, C\" binds\n(answer: B gets assigned to A and then the whole thing yields C).\n\nI wonder if\n\n\treturn (errno = ENOSYS), -1;\n\nis accepted by the compiler, but in these error handling we do not\nhave to be cute, and updated code that is both simple and stupid\nreads very well.\n\nWill queue. Thanks.\n\n"},{"id":"530894","messageId":"20251118094918.GC530545@coredump.intra.peff.net","threadId":"64499","inReplyTo":"xmqqy0o4gv99.fsf@gitster.g","subject":"Re: [PATCH] mingw: avoid the comma operator","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-18T09:49:18Z","receivedAt":"2025-11-18T09:49:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 17, 2025 at 02:16:34PM -0800, Junio C Hamano wrote:\n\n> > \t/* only these flags are supported */\n> > -\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))\n> > -\t\treturn errno = ENOSYS, -1;\n> > +\tif ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND)) {\n> > +\t\terrno = ENOSYS;\n> > +\t\treturn -1;\n> > +\t}\n> \n> Good riddance. It indeed is somewhat hard to read, especially\n> because it may not be apparent to readers how \"A = B, C\" binds\n> (answer: B gets assigned to A and then the whole thing yields C).\n> \n> I wonder if\n> \n> \treturn (errno = ENOSYS), -1;\n> \n> is accepted by the compiler, but in these error handling we do not\n> have to be cute, and updated code that is both simple and stupid\n> reads very well.\n\nAgreed that we are best avoiding comma operators when we can. There is\none spot where we use it, though, and I haven't figured out a good way\naround it: in the error() macro wrapper.\n\nI guess it does not cause the same compiler complaints because there is\nno assignment in it. So if nobody is complaining, we can just avert our\neyes when looking at the macro. :)\n\n-Peff\n"}]}