[PATCH v3 00/10] Avoid the comma operator

["Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>]

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 <Johannes.Schindelin@gmx.de>
      
       ## 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 <phillip.wood123@gmail.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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 <Johannes.Schindelin@gmx.de>
      
       ## 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 <phillip.wood@dunelm.org.uk>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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 <ps@pks.im>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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 <sunshine@sunshineco.com>
          Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
      
       ## 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