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

Corinna Vinschen wrote:
> On Nov  8 15:37, Corinna Vinschen wrote:
>> 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...
> So then, what about
>
>    if (l[dp]-U'1'<9U && r[dp]-U'1'<9U) {
>
> At least
>
>    static_assert(std::is_same_v<decltype(l[dp]-U'1'), unsigned int>);
>
> is happy.
>

In practice, this should work for all existing use cases of newlib. In 
theory, it is not generally portable:

U'1' has type char32_t (C11) which is unsigned. If the very rare ILP64 
data model would be used, this would be promoted to 64-bit signed int.

This variant does not depend on signedness or integer overflow:

   if ('1' <= l[dp] && l[dp] <= '9' && '1' <= r[dp] && r[dp] <= '9') {