Re: [PATCH v2 4/5] wildmatch: use char instead of uchar
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 27, 2023, 20:07 UTC
- Message-ID
- <xmqqh6v6swi6.fsf@gitster.g>
- In-Reply-To
- <20230226115021.1681834-5-masahiroy@kernel.org>
Masahiro Yamada <masahiroy@kernel.org> writes:
Show 7 quoted lines
> GIT has its own implementations, so the behavior is clear.
>
> In fact, commit 4546738b58a0 ("Unlocalized isspace and friends")
> says one of the motivations is "we want the right signed behaviour".
>
> sane_istest() casts the given character to (unsigned char) anyway
> before sane_ctype[] table lookup, so dowild() can use 'char'.Use of values taken from a char/uchar in sane_istest() is designed to be safe, and between using char*/uchar* to scan pieces of memory would not make much difference, so changes like ...
> case '*':
> if (*++p == '*') {
> - const uchar *prev_p = p - 2;
> + const char *prev_p = p - 2;... this is safe, and so is ...
> - const char *slash = strchr((char*)text, '/'); > + const char *slash = strchr(text, '/');
... this.
But does the comparison between t_ch_upper and prev_ch behave the same with and without this patch?
> - uchar t_ch_upper = toupper(t_ch); > + char t_ch_upper = toupper(t_ch); > if (t_ch_upper <= p_ch && t_ch_upper >= prev_ch)
Here t_ch is already known to pass islower(), so t_ch_upper would be within a reasonable range and "char" should be able to store it safely without its value wrapping around to negative. Do we have a similar guarantee on prev_ch, or can it be more than 128, which would have caused the original to say "t_ch_upper is smaller than prev_ch" but now because prev_ch can wrap around to a negative value, leading the conditional to behave differently, or something?
Show 16 quoted lines
> matched = 1;
> }
> p_ch = 0; /* This makes "prev_ch" get set to 0. */
> } else if (p_ch == '[' && p[1] == ':') {
> - const uchar *s;
> + const char *s;
> int i;
> for (s = p += 2; (p_ch = *p) && p_ch != ']'; p++) {} /*SHARED ITERATOR*/
> if (!p_ch)
> @@ -237,5 +235,5 @@ static int dowild(const uchar *p, const uchar *text, unsigned int flags)
> /* Match the "pattern" against the "text" string. */
> int wildmatch(const char *pattern, const char *text, unsigned int flags)
> {
> - return dowild((const uchar*)pattern, (const uchar*)text, flags);
> + return dowild(pattern, text, flags);
> }