Avoid the comma operator

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 <[email protected]>
      
       ## 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 <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## 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 <[email protected]>

## 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 <[email protected]>

## 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 <[email protected]>

## 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 <[email protected]>
          Signed-off-by: Johannes Schindelin <[email protected]>
      
       ## 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