diff mbox series

[v2,08/10] compat/regex: explicitly mark intentional use of the comma operator

Message ID dc626f36df34df4897289e508dbf608512a93870.1742945534.git.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series Avoid the comma operator | expand

Commit Message

Johannes Schindelin March 25, 2025, 11:32 p.m. UTC 
From: Johannes Schindelin <johannes.schindelin@gmx.de>

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.

In the `compat/regex/` code, the comma operator is used twice, once to
avoid surrounding two conditional statements with curly brackets, the
other one to increment two counters simultaneously in a `do ... while`
condition.

The first one is replaced with a proper conditional block, surrounded by
curly brackets.

The second one would be harder to replace because the loop contains two
`continue`s. Therefore, the second one is marked as intentional by
casting the value-to-discard to `void`.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 compat/regex/regex_internal.c | 7 ++++---
 compat/regex/regexec.c        | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

Comments

Taylor Blau March 26, 2025, 8:35 p.m. UTC | #1 
On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:
> diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
> index ec5cc5d2dd1..7672583bf7e 100644
> --- a/compat/regex/regex_internal.c
> +++ b/compat/regex/regex_internal.c
> @@ -1231,9 +1231,10 @@ 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])
> -	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])

Should the other arms of this conditional have matching curly-braces?

Thanks,
Taylor
Johannes Schindelin March 27, 2025, 10:29 a.m. UTC | #2 
Hi Taylor,

On Wed, 26 Mar 2025, Taylor Blau wrote:

> On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
> > index ec5cc5d2dd1..7672583bf7e 100644
> > --- a/compat/regex/regex_internal.c
> > +++ b/compat/regex/regex_internal.c
> > @@ -1231,9 +1231,10 @@ 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])
> > -	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])
>
> Should the other arms of this conditional have matching curly-braces?

No. Have a look around in that file, that's not the coding convention.

However, a valid concern is that I used Git's convention for the curly
brackets (appending the open bracket to the `if` line), which is _not_ the
convention used in this file, which uses GNU conventions instead. I will
address that concern in the next iteration.

Ciao,
Johannes
Taylor Blau March 27, 2025, 9:51 p.m. UTC | #3 
On Thu, Mar 27, 2025 at 11:29:16AM +0100, Johannes Schindelin wrote:
> Hi Taylor,
>
> On Wed, 26 Mar 2025, Taylor Blau wrote:
>
> > On Tue, Mar 25, 2025 at 11:32:12PM +0000, Johannes Schindelin via GitGitGadget wrote:
> > > diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
> > > index ec5cc5d2dd1..7672583bf7e 100644
> > > --- a/compat/regex/regex_internal.c
> > > +++ b/compat/regex/regex_internal.c
> > > @@ -1231,9 +1231,10 @@ 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])
> > > -	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])
> >
> > Should the other arms of this conditional have matching curly-braces?
>
> No. Have a look around in that file, that's not the coding convention.

I was just about to respond that even though it breaks the convention,
that we should encourage good hygeine by ensuring new code follows the
CodingGuidelines, even if it looks wonky in the context of the rest of
the file.

But this is compat/regex code, which clearly does not need to follow the
convention. Sorry about that!

Thanks,
Taylor
diff mbox series

Patch

diff --git a/compat/regex/regex_internal.c b/compat/regex/regex_internal.c
index ec5cc5d2dd1..7672583bf7e 100644
--- a/compat/regex/regex_internal.c
+++ b/compat/regex/regex_internal.c
@@ -1231,9 +1231,10 @@  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])
-	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])
 	dest->elems[--sbase] = src->elems[is--];
       else /* if (dest->elems[id] > src->elems[is]) */
 	--id;
diff --git a/compat/regex/regexec.c b/compat/regex/regexec.c
index 2eeec82f407..c08f1bbe1f5 100644
--- a/compat/regex/regexec.c
+++ b/compat/regex/regexec.c
@@ -2210,7 +2210,7 @@  sift_states_bkref (const re_match_context_t *mctx, re_sift_context_t *sctx,
 	  /* mctx->bkref_ents may have changed, reload the pointer.  */
 	  entry = mctx->bkref_ents + enabled_idx;
 	}
-      while (enabled_idx++, entry++->more);
+      while ((void)enabled_idx++, entry++->more);
     }
   err = REG_NOERROR;
  free_return: