Re: [PATCH] lib/strtoul: fix MISRA R10.2 violation

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 15 May 2024, Jan Beulich wrote:
> 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))

Oh I see. This is much more similar to the original suggestion from
Bugseng. Let me send a v2.