bit testing nervousness...

bit testing nervousness...

[Jeff Garzik]

Hey...

WRT all these test_bit()/set_bit() cleanups.   I am a bit nervous about
these changes that are coming in...

When I see types change from "u8" or "u32" to "long" just to make
<foo>_bit() work, that really makes me think that cleanup is wrong.  I
haven't looked closely at the recent set_bit() cleanups yet, but I am
willing to bet that at least some of them are wrongly changing the size
of a variable's type.

My preference would be to _eliminate_ the set_bit call and simply
open-code the bitop, i.e.
	set_bit(bitnum, &foo);
become
	foo |= (1 << bitnum);
	
Really, for each cleanup, you need to look hard at the change and
see if <foo>_bit() is being used for atomicity reasons or simply
programmer preference.  (and other issues like endian issues)  The
latter can easily be changed to open-coding.

Disclaimer, my argument is null and void if each change has been closely
studied and is really correct :)  However I'm guessing we all are only
glancing at the changes :)

	Jeff

Re: bit testing nervousness...

[Arnaldo Carvalho de Melo]

Em Sun, Dec 08, 2002 at 10:54:00PM -0500, Jeff Garzik escreveu:
> Hey...
> 
> WRT all these test_bit()/set_bit() cleanups.   I am a bit nervous about
> these changes that are coming in...

lets try to calm you then 8)
 
> When I see types change from "u8" or "u32" to "long" just to make
> <foo>_bit() work, that really makes me think that cleanup is wrong.  I
> haven't looked closely at the recent set_bit() cleanups yet, but I am
> willing to bet that at least some of them are wrongly changing the size
> of a variable's type.
> 
> My preference would be to _eliminate_ the set_bit call and simply
> open-code the bitop, i.e.
> 	set_bit(bitnum, &foo);
> become
> 	foo |= (1 << bitnum);

I think this can be a good idea, but in some cases, like the set_rx_mode
routines (multicast) it depends on the conversion to long, so those ones
should be dealt with in a different fashion, BTW, I haven't touched those
ones.
 	
> Really, for each cleanup, you need to look hard at the change and
> see if <foo>_bit() is being used for atomicity reasons or simply
> programmer preference.  (and other issues like endian issues)  The
> latter can easily be changed to open-coding.
> 
> Disclaimer, my argument is null and void if each change has been closely
> studied and is really correct :)  However I'm guessing we all are only
> glancing at the changes :)

Lets see:

o drivers/atm/ambassador.c (ChangeSet@1.797.108.1)
o drivers/atm/horizon.c (ChangeSet@1.797.108.2)
o drivers/char/sx.c (ChangeSet@1.797.108.3)
o drivers/net/lance.c (ChangeSet@1.831.1.15)
o drivers/net/ni65.c (ChangeSet@1.797.108.8)
o drivers/net/dl2k.c (ChangeSet@1.831.1.34, ChangeSet@1.831.1.32)
o drivers/net/wan/sdla_fr.c (ChangeSet@1.831.1.41)
o include/linux/if_wanpipe_common.h (ChangeSet@1.831.1.42)

only sets/resets/tests a few bits, safe, no code depends on it changing
its size from 32 to 64 bits.

Humm, I was expecting a second type of changes, but I think all are safe,
even on a second glance. :-)

- Arnaldo