Re: [PATCH 4/5 ver2] rt2x00: Compiler warning unmasked by fix of BUILD_BUG_ON

[Ivo van Doorn]

On Monday 01 September 2008, Boaz Harrosh wrote:
> Ivo Van Doorn wrote:
> > On Mon, Sep 1, 2008 at 3:44 PM, Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
> >> A "Set" of a sign-bit in an "&" operation causes a compiler warning.
> >> Make calculations unsigned.
> >>
> >> [ The warning was masked by the use of (void)() cast in the old
> >>  BUILD_BUG_ON() ]
> >>
> >> Signed-off-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx>
> >> TO: Ivo van Doorn <IvDoorn@xxxxxxxxx>
> >> TO: John W. Linville <linville@xxxxxxxxxxxxx>
> >> CC: Ingo Molnar <mingo@xxxxxxx>
> >> CC: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> >> ---
> >>  drivers/net/wireless/rt2x00/rt2x00reg.h |    4 ++--
> >>  1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> index 7e88ce5..e71b793 100644
> >> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> >> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
> >>  */
> >>  #define is_power_of_two(x)     ( !((x) & ((x)-1)) )
> >>  #define low_bit_mask(x)                ( ((x)-1) & ~(x) )
> >> -#define is_valid_mask(x)       is_power_of_two(1 + (x) + low_bit_mask(x))
> >> -
> >> +#define is_valid_mask(x)       is_power_of_two(1 + (x) + \
> >> +                                       low_bit_mask((unsigned long)x))
> > 
> > I know I typed it wrong, but you are missing the unsigned long cast for the
> > is_power_of_two argument here (Which could also be done in the
> > is_valid_mask() definition).
> > 
> 
> I thought you suggested that on purpose. Since at the end it is all
> one expression, the compiler propagates the cast to all participants.
> 
> Do you want that I send a fix for readability's sake?

Yes, thanks.

> >>  /*
> >>  * Macro's to find first set bit in a variable.
> >>  * These macro's behaves the same as the __ffs() function with
> >> --
> >> 1.5.6.rc1.5.gadf6
> >>
> 
> Is below what you mean? but if so then perhaps my original one is
> clearer. Note that it compiles and works as is.

Below patch is good. Your original one would have had the same comment
as the original one, where the cast was only done on 1 x instead of both usages.

I think the below version is the most clear solution. :)

Ivo

> ---
> diff --git a/drivers/net/wireless/rt2x00/rt2x00reg.h b/drivers/net/wireless/rt2x00/rt2x00reg.h
> index 7e88ce5..e71b793 100644
> --- a/drivers/net/wireless/rt2x00/rt2x00reg.h
> +++ b/drivers/net/wireless/rt2x00/rt2x00reg.h
> @@ -136,8 +136,8 @@ struct rt2x00_field32 {
>   */
>  #define is_power_of_two(x)	( !((x) & ((x)-1)) )
>  #define low_bit_mask(x)		( ((x)-1) & ~(x) )
> -#define is_valid_mask(x)	is_power_of_two(1 + (x) + low_bit_mask(x))
> -
> +#define is_valid_mask(x)	is_power_of_two(1LU + (unsigned long)(x) + \
> +					low_bit_mask((unsigned long)x))
>  /*
>   * Macro's to find first set bit in a variable.
>   * These macro's behaves the same as the __ffs() function with
> -- 1.5.6.rc1.5.gadf6 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ 
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/