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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 10, 2023, 19:10 UTC
Message-ID: <xmqqo7q1ibek.fsf@gitster.g>
In-Reply-To: <20230210075939.44949-2-masahiroy@kernel.org>

Masahiro Yamada <masahiroy@kernel.org> writes:

> Use them with care because they are not robust against the pointer
> increment, like isblank(*s++).
>
> The same issue already exists for isspace().

Does it?

>  #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)

sane_istest() evaluates x and mask only once, and isspace evaluates x only once, no?

	isspace(*x++)
	->
	sane_istest(*x++,GIT_SPACE)
	->
	((sane_ctype[(unsigned char)(*x++)] & GIT_SPACE) != 0)

> +#define isblank(x) (isspace(x) || (x) == '\t')
> +#define isgraph(x) (isprint(x) && !isspace(x))

Given that all the other tests are implemented with just picking an integer from the table and checking bit(s), the above two look somewhat out of place. The fact, as you noted, that they cannot be used safely does not help, either.