Re: [ath9k-devel] [RFC] ath9k_hw: precedence bug in ath9k_hw_set_ofdm_nil()

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Tue, Apr 10, 2012 at 11:58:26AM +0200, Felix Fietkau wrote:
> >  	if ((aniState->noiseFloor >= aniState->rssiThrHigh) &&
> > -	    (!aniState->ofdmWeakSigDetectOff !=
> > +	    (aniState->ofdmWeakSigDetectOff !=
> Looking at other Atheros code, I think this patch is wrong, it should
> be: aniState->ofdmWeakSigDetectOff == entry_ofdm->ofdm_weak_signal_on
> 
> While a bit confusing, the behavior of the original code was correct,
> aniState->ofdmWeakSigDetectOff is used as a boolean.
> 
> This code badly needs a cleanup, the whole mess with *on vs *off
> variables is quite confusing.
> 

Yep.  It's bad to name variables xxx_off because it leads triple
negatives like we see here.

I guess that "if (!off != on) { ..." might be more readable than
"if (off == on) { ...".  There are a couple ways to silence this
warning in Smatch.

The first way would be to add parenthesis like,
"((!off) != on) {...".  I think it helps because it makes the negate
stand out and you don't have to think about how the precedence
rules works.  Other people think it's a needless parenthesis for
something that is obvious.

The other way would be to make either ->ofdmWeakSigDetectOff or
->ofdm_weak_signal_on be type bool.  I think this would make the
code clearer.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html