On Tuesday, April 21, 2015 6:35 AM, Dan Carpenter wrote: > On Tue, Apr 21, 2015 at 01:52:09PM +0100, Ian Abbott wrote: >>>Is that really an improvement? The original code was actually defined. >>> >>>1U << 32 is actually defined. It is zero. Which works for us. >> >> According to the C standards it is undefined (for 32-bit unsigned >> int). See the late draft of ISO/IEC 9899:2011 (dubbed C11) at >> <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1570.pdf>, §6.5.7, >> Semantics: >> >> 3. The integer promotions are performed on each of the operands. The >> type of the result is that of the promoted left operand.If the value >> of the right operand is negative or is greater than or equal to the >> width of the promoted left operand, the behavior is undefined. >> > > Yeah. You're right. > >> >> It would be better to use (1U << b_chans) instead of (1 << b_chans) >> in the code above, or possibly the slightly more obtuse: >> >> b_mask = ((b_chans < 32) ? 1 << b_chans : 0) - 1; > > I like just switching Hartley's 1 to 1U. A quick test: #include <stdio.h> int main(int argc, char *argv[]) { int shift; shift = 31; printf("(1 << %d) - 1 = %x\n", shift, (1 << shift) - 1); printf("(1U << %d) - 1 = %x\n", shift, (1U << shift) - 1); shift = 32; printf("(1 << %d) - 1 = %x\n", shift, (1 << shift) - 1); printf("(1U << %d) - 1 = %x\n", shift, (1U << shift) - 1); return 0; } Produces this result: (1 << 31) - 1 = 7fffffff (1U << 31) - 1 = 7fffffff (1 << 32) - 1 = 0 (1U << 32) - 1 = 0 Looks like the 1 vs 1U doesn't make any difference. And a shift of 32 results in the wrong value. Are you ok with the original patch? Regards, Hartley _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
