Re: [PATCH 3/5] wildmatch: remove NEGATE_CLASS and NEGATE_CLASS2 macros

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Feb 10, 2023, 13:11 UTC
Message-ID: <230210.865yc9wtkx.gmgdl@evledraar.gmail.com>
In-Reply-To: <20230210075939.44949-4-masahiroy@kernel.org>

On Fri, Feb 10 2023, Masahiro Yamada wrote:

Show 13 quoted lines

> The other glob patterns are hard-coded in dowild(). Do likewise.
> [...]
> diff --git a/wildmatch.c b/wildmatch.c
> index a510b3fd23..93800b8eac 100644
> --- a/wildmatch.c
> +++ b/wildmatch.c
> @@ -14,10 +14,6 @@
>  
>  typedef unsigned char uchar;
>  
> -/* What character marks an inverted character class? */
> -#define NEGATE_CLASS	'!'
> -#define NEGATE_CLASS2	'^'

Thanks, maybe these made sense in rsync's codebase, but...

Show 7 quoted lines

>  #define CC_EQ(class, len, litmatch) ((len) == sizeof (litmatch)-1 \
>  				    && *(class) == *(litmatch) \
>  				    && strncmp((char*)class, litmatch, len) == 0)
> @@ -137,12 +133,8 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
>  			return WM_ABORT_ALL;
>  		case '[':
>  			p_ch = *++p;

...as the context shows we hardcode most of these tokens.

> -#ifdef NEGATE_CLASS2
> -			if (p_ch == NEGATE_CLASS2)
> -				p_ch = NEGATE_CLASS;
> -#endif

Hrm, but isn't this a logic error? No it's not, because...

>  			/* Assign literal 1/0 because of "matched" comparison. */
> -			negated = p_ch == NEGATE_CLASS ? 1 : 0;
> +			negated = p_ch == '!' || p_ch == '^' ? 1 : 0;

...you're refcatoring thise while at it.

Personally I'd prefer to just see this change without this, and it came as a surprise given the commit message.

I find the pre-image easier to read (sans the macro check, which you should be removing).