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
