Re: [patch] oxygen: clean up. make precedence explicit

[Bernd Petrovitsch <bernd@petrovitsch.priv.at>]

On Fre, 2010-02-19 at 13:10 +0300, Dan Carpenter wrote:
> On Fri, Feb 19, 2010 at 09:29:05AM +0100, Clemens Ladisch wrote:
> > > This doesn't change anything, but I think it makes the code clearer.
> > > It silences a smatch warning:
> > > sound/pci/oxygen/oxygen_mixer.c +91 dac_mute_put(7) warn: add some parenthesis here?
> > 
> > That message doesn't say why some parentheses should be added.
> > And it's a question; how do I give it the answer "no"?  :-)
> > 
> > > -	changed = !value->value.integer.value[0] != chip->dac_mute;
> > > +	changed = (!value->value.integer.value[0]) != chip->dac_mute;
> > 
> > This doesn't look any clearer to me; I don't think that the unary
> > negation operator could be thought to have lower precedence than "!=".
> 
> Well, it's hard to argue that it's more ambiguous.  :P
But it doesn't make the code clearer - unless you are a C novice. Unary
operators generally bind stronger than others - be it "+", "-", "!",
"~", "*".
I would expect kernel programmers to know that (and I don't assume
in-depth knowledge of operator precedence rules).

> > Why does smatch warn about this combination?  Do such errors actually
> > happen:
> 
> Yep.  I have made some myself when writing smatch.
> 
> For example here are some related bugs in the current kernel.
> 
> drivers/staging/rtl8192u/ieee80211/ieee80211_wx.c
>    721          if (!ext->ext_flags & IW_ENCODE_EXT_GROUP_KEY &&
Well, I see potential bugs here and the if() should have been
a) if (!(ext->ext_flags & IW_ENCODE_EXT_GROUP_KEY) &&
b) if (!ext->ext_flags && IW_ENCODE_EXT_GROUP_KEY &&
So you one has to look at the driver for the correct fix (and perhaps
both of above are wrong).

And I don't see what parenthesis around a logical negations can help
with the above error example.

	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at