> Date: Mon, 11 Oct 2010 09:47:56 +0200 > From: Takashi Iwai <[email protected]> > > At Fri, 08 Oct 2010 23:09:26 +0200, > Henrik Rydberg wrote: > > > > On 10/08/2010 10:51 PM, Mark Kettenis wrote: > > > > >> Date: Fri, 08 Oct 2010 21:36:02 +0200 > > >> From: Takashi Iwai <[email protected]> > > >> > > >> At Fri, 8 Oct 2010 20:48:10 +0200 (CEST), > > >> Mark Kettenis wrote: > > >>> > > >>>> From: Takashi Iwai <[email protected]> > > >>>> Date: Fri, 8 Oct 2010 19:22:29 +0200 > > >>> > > >>> Sorry, but it isn't obvious to me what issue this fixes. > > >> > > >> In C, "1" is an integer, not an unsigned long. > > >> Thus (1 << 33) doesn't give you the 33th bit shift, but it's undefined. > > > > > > But if you'd actually use 33 (or event 32) as an offset, things > > > wouldn't work on a 32-bit platform would they? > > > > > > I believe this is what the patch is supposed to fix. > > Yep :) > > > > > Anyway, > > > > > >> If any, this must be (1UL << 32). > > > > > > This is the idiom that is much more common. I probably wouldn't have > > > questioned things if you'd written it like that. I recommend you to > > > stick to this version. > > > > > > For a counter-example, see the definition of test_bit() in the Linux kernel. > > > > >> Also, it'd be better if such a test macro returns 1 instead of a > > >> random non-zero value. So does my patch. > > > > > > In C this doesn't matter. > > > > > > It is still useful to know if a logic expression evaluates to (0, 1) or (0, > > nonzero). > > It does matter if the returned type is bigger. > Try an example code below on 32bit and 64bit machines: > > ================================================================ > #include <stdio.h> > > #define LONG_BITS (sizeof(long) * 8) > #define OFF(x) ((x) % LONG_BITS) > #define LONG(x) ((x) / LONG_BITS) > > #define TEST_BIT1(bit, array) (array[LONG(bit)] & (1 << OFF(bit))) > #define TEST_BIT2(bit, array) (array[LONG(bit)] & (1UL << OFF(bit))) > #define TEST_BIT3(bit, array) ((array[LONG(bit)] >> OFF(bit)) & 1) > > static unsigned long test[4]; > > int main() > { > const int bit = 33; > int result; > > /* set the bit */ > test[LONG(bit)] |= 1UL << OFF(bit); > > result = TEST_BIT1(bit, test); > printf("TEST1 %d = %d\n", bit, result); > > result = TEST_BIT2(bit, test); > printf("TEST2 %d = %d\n", bit, result); > > result = TEST_BIT3(bit, test); > printf("TEST3 %d = %d\n", bit, result); > > return 0; > } > ================================================================ > > On a 32bit machine, it results in: > TEST1 33 = 2 > TEST2 33 = 2 > TEST3 33 = 1 > > while on a 64bit machine, it results in: > TEST1 33 = 0 > TEST2 33 = 0 > TEST3 33 = 1 > > See TEST2 still failed although it uses 1UL.
This example is meaningless. On every reasonable 32-bit architecture, long is a 32-bit integer. So > /* set the bit */ > test[LONG(bit)] |= 1UL << OFF(bit); when bit is 33 (or even 32), is completely undefined on a 32-bit machine. Therefore, you should never call TEST_BIT() with a first argument of 32 or larger, even on a 64-bit architecture, since the code wouldn't work on a 32-bit machine. One can even argue that for this reason the change you're suggesting is dangerous, since people writing code on a 64-bit machine might not notice that they're writing coding that won't run on a 32-bit machine. _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
