Re: [PATCH] Add cast to unsigned char to strverscmp

On Nov  8 12:41, Christian Franke wrote:
> Corinna Vinschen wrote:
> > On Nov  7 13:58, Joel Sherrill wrote:
> > > On Thu, Nov 7, 2024 at 1:06 PM <[email protected]> wrote:
> > > 
> > > > On 2024-11-07 09:53, Jeremy Bettis wrote:
> > > > > GCC 14.2 with -Werror=sign-compare fails on this code.
> > > > > 
> > > > > Signed-off-by: Jeremy Bettis <[email protected] <mailto:
> > > > [email protected]>>
> > > > > ---
> > > > >    newlib/libc/string/strverscmp.c | 2 +-
> > > > >    1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/newlib/libc/string/strverscmp.c
> > > > b/newlib/libc/string/strverscmp.c
> > > > > index 55966335f..e86718faa 100644
> > > > > --- a/newlib/libc/string/strverscmp.c
> > > > > +++ b/newlib/libc/string/strverscmp.c
> > > > > @@ -76,7 +76,7 @@ int strverscmp(const char *l0, const char *r0)
> > > > >     else if (c!='0') z=0;
> > > > >     }
> > > > > 
> > > > > - if (l[dp]<'1' + 9U && r[dp]<'1' + 9U) {
> > I'm puzzled.  This doesn't look like newlib code.  Newlib's code looks
> > like this:
> > 
> >          if (l[dp]-'1'<9U && r[dp]-'1'<9U) {
> > 
> > See https://sourceware.org/git/?p=newlib-cygwin.git;a=blob;f=newlib/libc/string/strverscmp.c;hb=HEAD#l79
> > 
> > Given that l and r are unsigned char anyway, the entire expression
> > is unsigned, so there shouldn't be a sign-compare error in newlib's
> > version.
> > 
> 
> It's actually signed, IIRC due to changing 'unsigned preserving' (K&R C) to
> 'value preserving' (C89) implicit conversions.
> 
> "Proof" using C++17 compile time checks:
> 
> $ cat uchar2int.cc
> #include <cstddef>
> #include <type_traits>
> 
> const unsigned char *l;
> std::size_t dp;
> 
> static_assert(std::is_same_v<decltype(l[dp]), const unsigned char &>);
> static_assert(std::is_same_v<decltype(l[dp]-'1'), int>);
> static_assert(std::is_same_v<decltype(l[dp]-0x31U), unsigned int>);
> 
> $ g++ -S -Wall -W uchar2int.cc
> [no failed assert]

Drat.  Looks like I'm still living in K&R country...


Corinna