mbox series

[v2,00/10] Avoid the comma operator

Message ID pull.1889.v2.git.1742945534.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Avoid the comma operator | expand

Message

Elijah Newren via GitGitGadget March 25, 2025, 11:32 p.m. UTC
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
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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

Comments

Patrick Steinhardt March 26, 2025, 5:54 a.m. UTC | #1
On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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).

Thanks for all of these fixes!

Patrick
Jeff King March 26, 2025, 5:50 p.m. UTC | #2
On Tue, Mar 25, 2025 at 11:32:04PM +0000, Johannes Schindelin via GitGitGadget wrote:

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

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