[patch] oxygen: clean up. make precedence explicit

[patch] oxygen: clean up. make precedence explicit

[Dan Carpenter]

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?

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
Compile tested.

diff --git a/sound/pci/oxygen/oxygen_mixer.c b/sound/pci/oxygen/oxygen_mixer.c
index f375b8a..28590b9 100644
--- a/sound/pci/oxygen/oxygen_mixer.c
+++ b/sound/pci/oxygen/oxygen_mixer.c
@@ -88,7 +88,7 @@ static int dac_mute_put(struct snd_kcontrol *ctl,
 	int changed;
 
 	mutex_lock(&chip->mutex);
-	changed = !value->value.integer.value[0] != chip->dac_mute;
+	changed = (!value->value.integer.value[0]) != chip->dac_mute;
 	if (changed) {
 		chip->dac_mute = !value->value.integer.value[0];
 		chip->model.update_dac_mute(chip);

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

[Clemens Ladisch]

> 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 "!=".

Why does smatch warn about this combination?  Do such errors actually
happen?


Regards,
Clemens

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

[Dan Carpenter]

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

> 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 &&
   722              ext->alg != IW_ENCODE_ALG_WEP)
   723                  if (idx != 0 || ieee->iw_mode != IW_MODE_INFRA)
   724                          return -EINVAL;

                        if (!j->dsp.low != 0x20) {

drivers/telephony/ixj.c
  6834                          if (!j->dsp.low != 0x20) {

Mostly the real bugs have been fixed now.  Roel Kluin fixed quite a 
few of these before and I've fixed a couple.

regards,
dan carpenter

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

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

[Dan Carpenter]

On Fri, Feb 19, 2010 at 11:33:30AM +0100, Bernd Petrovitsch wrote:
> 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) &&

Yep.  This is clearly what the code should say.

The problem in the original code is that IW_ENCODE_EXT_GROUP_KEY is not 
equal to either 1 or to 0.  (So that means the condition in the original 
code is always false).

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

Basically often when people write:
	if (!foo == bar) { ...

What they mean is:
	if (!(foo == bar)) { ...

But if they really do mean the original code they could just write 
this so it's clear to everyone: 
	if ((!foo) == bar) { ...

To me it's like "==" vs "=".  Of course, every programmer knows the 
what the difference is but it helps to have gcc warn about adding the 
extra parenthesis.  Maybe I suck, but it definitely has helped me in 
then past.

I don't have strong feelings about this btw.  The original code in
oxygyn_mixer works fine.  I just was making some changes to smatch and
that was a new warning today.  There is no method to my madness.

regards,
dan carpenter


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

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

[Dan Carpenter]

On Fri, Feb 19, 2010 at 02:29:21PM +0300, Dan Carpenter wrote:
> On Fri, Feb 19, 2010 at 11:33:30AM +0100, Bernd Petrovitsch wrote:
> > 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) &&
> 
> Yep.  This is clearly what the code should say.
> 
> The problem in the original code is that IW_ENCODE_EXT_GROUP_KEY is not 
> equal to either 1 or to 0.  (So that means the condition in the original 
> code is always false).
> 

Except the last _bit_ of IW_ENCODE_EXT_GROUP_KEY _is_ equal to one or zero
and I am an idiot.

But still, the original code here is wrong and your example code is 
correct.

regards,
dan carpenter

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

[Clemens Ladisch]

Bernd Petrovitsch wrote:
> On Fre, 2010-02-19 at 14:29 +0300, Dan Carpenter wrote:
> > On Fri, Feb 19, 2010 at 11:33:30AM +0100, Bernd Petrovitsch wrote:
> > Basically often when people write:
> > 	if (!foo == bar) { ...
> > 
> > What they mean is:
> > 	if (!(foo == bar)) { ...

But there are also cases where they mean what they've written.

> Ugh. The IMHO better way is 
> 	if (foo != bar) { ...

In my case, the driver compares an "enabled" variable against a
"disabled" one; negating the comparison operator would obfuscate the
logic.

> > But if they really do mean the original code they could just write 
> > this so it's clear to everyone: 
> > 	if ((!foo) == bar) { ...

This is unnatural (especially in a simple example like this) because
the parens haven't been needed at all before smatch.


!foo==bar is always identical to !(foo==bar) for boolean values; to
avoid false positives, you could output the warning only when the code
is trying to manipulate non-boolean values.  IMO the message would be
justified if it said "using suspicious boolean operations on non-boolean
types".  (In fact, my driver uses types long and u8 in this expression,
so I will clean it up.)


Regards,
Clemens

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

[Dan Carpenter]

On Fri, Feb 19, 2010 at 06:24:10PM +0100, Clemens Ladisch wrote:
> Bernd Petrovitsch wrote:
> > On Fre, 2010-02-19 at 14:29 +0300, Dan Carpenter wrote:
> > > On Fri, Feb 19, 2010 at 11:33:30AM +0100, Bernd Petrovitsch wrote:
> > > Basically often when people write:
> > > 	if (!foo == bar) { ...
> > > 
> > > What they mean is:
> > > 	if (!(foo == bar)) { ...
> 
> But there are also cases where they mean what they've written.
> 
> > Ugh. The IMHO better way is 
> > 	if (foo != bar) { ...
> 
> In my case, the driver compares an "enabled" variable against a
> "disabled" one; negating the comparison operator would obfuscate the
> logic.
> 
> > > But if they really do mean the original code they could just write 
> > > this so it's clear to everyone: 
> > > 	if ((!foo) == bar) { ...
> 
> This is unnatural (especially in a simple example like this) because
> the parens haven't been needed at all before smatch.
> 
> 
> !foo==bar is always identical to !(foo==bar) for boolean values; to
> avoid false positives, you could output the warning only when the code
> is trying to manipulate non-boolean values.  IMO the message would be
> justified if it said "using suspicious boolean operations on non-boolean
> types".  (In fact, my driver uses types long and u8 in this expression,
> so I will clean it up.)
> 

Yup.  The check already takes the type into account.  Making chip->dac_mute
type bool would silence the message.

regards,
dan carpenter

> 
> Regards,
> Clemens