[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <55B54FD8.6020104@openwall.com>
Date: Mon, 27 Jul 2015 00:23:36 +0300
From: Alexander Cherepanov <ch3root@...nwall.com>
To: musl@...ts.openwall.com
Subject: Re: Left-shift of negative number
On 2015-07-26 19:53, Rich Felker wrote:
> On Sat, Jul 25, 2015 at 09:26:34PM +0300, Alexander Cherepanov wrote:
>>> I've gone ahead and made the change as commit
>>> fe7582f4f92152ab60e9523bf146fe28ceae51f6. If anything looks wrong,
>>> please let me know. Thanks again for the bug report.
>>
>> The new definition of R:
>>
>> #define R(a,b) ((uint32_t)((a==0x80 ? 0x40u-b : 0u-a) << 23))
>>
>> It implicitly casts a and b to unsigned (and triggers
>> -Wsign-conversion). Isn't it better to express it explicitly, e.g.
>
> Using implicit conversion to unsigned types is idiomatic in musl; we
Sorry, I didn't know it.
> don't use -Wsign-comparison or similar in the default CFLAGS. A
> discussion about changing that should be library-global, not specific
> to this one piece of code, but I'd rather not anyway. My view is that
> there's a tradeoff between using implicit conversions and casts: using
> the implicit conversions (and leaving off warnings for them) could
> hide bugs from wrong expectations about types, but using casts can
> hide much more dangerous conversions for which an implicit conversion
> would not even be possible. I generally like to keep the number of
> casts to a minimum.
It would be interesting to read when and if you have time to write about
it in more details.
>> by moving the cast to uint32_t inside the conditional operator? Or
>> maybe more intuitive to move the work with negative numbers outside
>> the conditional operator:
>>
>> #define R(a,b) (-(uint32_t)(a==0x80 ? b-0x40 : a) << 23)
>
> This actually would give the wrong result -- and strictly speaking,
> even worse, undefined behavior -- if uint32_t had lower rank than int.
> While other parts of musl assume 32-bit int for Linux-specific reasons
> (advanced parts of the futex API, etc.) I'd rather leave this code
> only assuming that int is >=32bit rather than exactly 32-bit.
Ok, thanks for the explanation.
--
Alexander Cherepanov
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
