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
