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

[Rajkumar Manoharan <rmanohar@xxxxxxxxxxxxxxxx>]

On Tue, Apr 10, 2012 at 01:57:25PM +0300, Dan Carpenter wrote:
> 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.
>
Declare them as bool type and rename one of them to common (either *_off or *_on).

-Rajkumar
--
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