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
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 ([email protected])
o drivers/atm/horizon.c ([email protected])
o drivers/char/sx.c ([email protected])
o drivers/net/lance.c ([email protected])
o drivers/net/ni65.c ([email protected])
o drivers/net/dl2k.c ([email protected], [email protected])
o drivers/net/wan/sdla_fr.c ([email protected])
o include/linux/if_wanpipe_common.h ([email protected])
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