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.