Date: Sun, 26 Jul 2015 12:53:25 -0400 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: Left-shift of negative number 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 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. > 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. Rich
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.