Re: [PATCH 1/5] git-compat-util: add isblank() and isgraph()

From: René Scharfe <l.s.r@web.de>
Date: Feb 10, 2023, 22:03 UTC
Message-ID: <654092a2-6d3e-7ab4-a747-1ce36daad03d@web.de>
In-Reply-To: <20230210075939.44949-2-masahiroy@kernel.org>

Am 10.02.23 um 08:59 schrieb Masahiro Yamada:

Show 39 quoted lines

> git-compat-util.h implements most of is*() macros.
>
> Add isblank() and isgraph(), which are useful to clean up wildmatch.c
> in a consistent way (in this and later commits).
>
> Use them with care because they are not robust against the pointer
> increment, like isblank(*s++).
>
> The same issue already exists for isspace().
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
>
>  git-compat-util.h |  4 ++++
>  wildmatch.c       | 14 ++------------
>  2 files changed, 6 insertions(+), 12 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 4f0028ce60..90b43b2bc9 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -1212,10 +1212,12 @@ extern const unsigned char tolower_trans_tbl[256];
>  /* Sane ctype - no locale, and works with signed chars */
>  #undef isascii
>  #undef isspace
> +#undef isblank
>  #undef isdigit
>  #undef isalpha
>  #undef isalnum
>  #undef isprint
> +#undef isgraph
>  #undef islower
>  #undef isupper
>  #undef tolower
> @@ -1236,10 +1238,12 @@ extern const unsigned char sane_ctype[256];
>  #define sane_istest(x,mask) ((sane_ctype[(unsigned char)(x)] & (mask)) != 0)
>  #define isascii(x) (((x) & ~0x7f) == 0)
>  #define isspace(x) sane_istest(x,GIT_SPACE)
> +#define isblank(x) (isspace(x) || (x) == '\t')

Our isspace() matches \t, \n, \r and space. A standard implementation would also match \v (vertical tab) and \f (form feed). Anyway, your isblank() here is the same as isspace() because the check for \t is redundant...

Show 20 quoted lines

>  #define isdigit(x) sane_istest(x,GIT_DIGIT)
>  #define isalpha(x) sane_istest(x,GIT_ALPHA)
>  #define isalnum(x) sane_istest(x,GIT_ALPHA | GIT_DIGIT)
>  #define isprint(x) ((x) >= 0x20 && (x) <= 0x7e)
> +#define isgraph(x) (isprint(x) && !isspace(x))
>  #define islower(x) sane_iscase(x, 1)
>  #define isupper(x) sane_iscase(x, 0)
>  #define is_glob_special(x) sane_istest(x,GIT_GLOB_SPECIAL)
> diff --git a/wildmatch.c b/wildmatch.c
> index 7e5a7ea1ea..85c4c7f8a7 100644
> --- a/wildmatch.c
> +++ b/wildmatch.c
> @@ -28,18 +28,8 @@ typedef unsigned char uchar;
>  # define ISASCII(c) isascii(c)
>  #endif
>
> -#ifdef isblank
> -# define ISBLANK(c) (ISASCII(c) && isblank(c))
> -#else
> -# define ISBLANK(c) ((c) == ' ' || (c) == '\t')

... and that's not the case for the original code, which only matches \t and space.

We already use eight bits for the lookup table values in sane_ctype. Perhaps it's time to move GIT_GLOB_SPECIAL, GIT_REGEX_SPECIAL, GIT_PATHSPEC_MAGIC and GIT_PUNCT to their own table to make room for flags for isprint(), isgraph() and isblank().

Show 13 quoted lines

> -#endif
> -
> -#ifdef isgraph
> -# define ISGRAPH(c) (ISASCII(c) && isgraph(c))
> -#else
> -# define ISGRAPH(c) (ISASCII(c) && isprint(c) && !isspace(c))
> -#endif
> -
> +#define ISBLANK(c) (ISASCII(c) && isblank(c))
> +#define ISGRAPH(c) (ISASCII(c) && isgraph(c))
>  #define ISPRINT(c) (ISASCII(c) && isprint(c))
>  #define ISDIGIT(c) (ISASCII(c) && isdigit(c))
>  #define ISALNUM(c) (ISASCII(c) && isalnum(c))