On 15.05.2024 00:52, Stefano Stabellini wrote:
> On Tue, 14 May 2024, Jan Beulich wrote:
>> On 14.05.2024 02:32, Stefano Stabellini wrote:
>>> Fix last violation of R10.2 by casting the result of toupper to plain
>>> char. Note that we don't want to change toupper itself as it is a legacy
>>> interface and it would cause more issues.
>>
>> Can you point me at a single example where a new issue would arise? All
>> places I've spotted (including tolower() uses) would appear to benefit
>> from changing toupper() / tolower() themselves. Further, since they are
>> both wrapper macros only anyway, if any concern remained, fiddling with
>> the wrapper macros while leaving alone the underlying inline functions
>> would allow any such use site to simply be switched to using the inline
>> functions directly. As said, from looking at it I don't expect that
>> would be necessary, so instead I'd rather hope that eventually we can
>> do away with the wrapper macros, renaming the inline functions
>> accordingly.
>
> If we change __toupper to return a plain char, then there are a few
> other things we need to change for consistency, see below. To be honest
> I thought it would cause more problems. I am OK to go with that if you
> all agree. (Nicola please have a look in case this introduces more
> issues elsewhere.)
>
>
> diff --git a/xen/include/xen/ctype.h b/xen/include/xen/ctype.h
> index 6dec944a37..6a6854e01c 100644
> --- a/xen/include/xen/ctype.h
> +++ b/xen/include/xen/ctype.h
> @@ -15,9 +15,9 @@
> #define _X 0x40 /* hex digit */
> #define _SP 0x80 /* hard space (0x20) */
>
> -extern const unsigned char _ctype[];
> +extern const char _ctype[];
Why would this be needed? I can't see a connection to toupper() / tolower().
> -#define __ismask(x) (_ctype[(int)(unsigned char)(x)])
> +#define __ismask(x) (_ctype[(int)(char)(x)])
This almost certainly is wrong. Whether plain char is signed or unsigned is
left to the compiler, and it being signed would result in possibly negative
array indexes. Again I can't see a connection to the issue at hand.
> @@ -34,14 +34,14 @@ extern const unsigned char _ctype[];
> #define isascii(c) (((unsigned char)(c))<=0x7f)
> #define toascii(c) (((unsigned char)(c))&0x7f)
>
> -static inline unsigned char __tolower(unsigned char c)
> +static inline char __tolower(char c)
> {
> if (isupper(c))
> c -= 'A'-'a';
> return c;
> }
>
> -static inline unsigned char __toupper(unsigned char c)
> +static inline char __toupper(char c)
> {
> if (islower(c))
> c -= 'a'-'A';
This isn't what I had suggested. First I said to leave alone the double-
underscore prefixed functions, and only touch the wrapper macros (as a
precaution in case any use site exists which relies on present behavior).
And then I didn't suggest to alter parameter types; only the return type
would need adjustment, I think, for what you're aiming at:
#define tolower(c) ((char)__tolower(c))
#define toupper(c) ((char)__toupper(c))
Jan
