Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org> wrote:

> From: Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org>
> 
> In commit e6bafba5b4765a5a252f1b8d31cbf6d2459da337, a bug was fixed 
> that involved converting !x & y to !(x & y).  The code below shows the 
> same pattern, and thus should perhaps be fixed in the same way.

>  	if (sta_ht_inf) {
>  		if ((!sta_ht_inf->ht_supported) ||
> -		   (!sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH))
> +		   (!(sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH)))
>  			return 0;

i'm wondering, could Sparse be extended to check for such patterns? 

People are regularly running "make C=1" and are sending fixes to make 
entire subsystems sparse-warning-free, so Sparse is a nice mechanism 
that works and it keeps code clean in the long run.

I dont think the "!X & Y" pattern is ever used legitimately [and even if 
it were used legitimately, it's easy to avoid the sparse false positive 
- while in the buggy case we have a clear bug].

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Christopher Li]

I think Al Viro has sent a patch to linux-sparse with subject
"[PATCH 3/3] catch !x & y brainos" does exactly that.

Chris



On Tue, Mar 4, 2008 at 10:38 PM, Ingo Molnar <mingo@elte.hu> wrote:
>
>  * Julia Lawall <julia@diku.dk> wrote:
>
>  > From: Julia Lawall <julia@diku.dk>
>  >
>  > In commit e6bafba5b4765a5a252f1b8d31cbf6d2459da337, a bug was fixed
>  > that involved converting !x & y to !(x & y).  The code below shows the
>  > same pattern, and thus should perhaps be fixed in the same way.
>
>
> >       if (sta_ht_inf) {
>  >               if ((!sta_ht_inf->ht_supported) ||
>  > -                (!sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH))
>  > +                (!(sta_ht_inf->cap & IEEE80211_HT_CAP_SUP_WIDTH)))
>  >                       return 0;
>
>  i'm wondering, could Sparse be extended to check for such patterns?
>
>  People are regularly running "make C=1" and are sending fixes to make
>  entire subsystems sparse-warning-free, so Sparse is a nice mechanism
>  that works and it keeps code clean in the long run.
>
>  I dont think the "!X & Y" pattern is ever used legitimately [and even if
>  it were used legitimately, it's easy to avoid the sparse false positive
>  - while in the buggy case we have a clear bug].
>
>         Ingo
>  --
>  To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
>
>
> the body of a message to majordomo@vger.kernel.org
>  More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Christopher Li <sparse@chrisli.org> wrote:

> I think Al Viro has sent a patch to linux-sparse with subject "[PATCH 
> 3/3] catch !x & y brainos" does exactly that.

ah - nice :-)

/me checks the linux-sparse archive

Al's patch is:

+                       if (op == '&' && expr->left->type == EXPR_PREOP &&
+                           expr->left->op == '!')
+                               warning(expr->pos, "dubious: !x & y");

i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?

	Ingo

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Harvey Harrison]

On Wed, 2008-03-05 at 08:02 +0100, Ingo Molnar wrote:
> * Christopher Li <sparse-55XgFHCVCFZAfugRpC6u6w@public.gmane.org> wrote:
> 
> > I think Al Viro has sent a patch to linux-sparse with subject "[PATCH 
> > 3/3] catch !x & y brainos" does exactly that.
> 
> ah - nice :-)
> 
> /me checks the linux-sparse archive
> 
> Al's patch is:
> 
> +                       if (op == '&' && expr->left->type == EXPR_PREOP &&
> +                           expr->left->op == '!')
> +                               warning(expr->pos, "dubious: !x & y");
> 
> i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
> 

Well, (!x & y) and (!x | y) are probably the two that might have been
intended otherwise.  (x & !y), (x | !y) are probably ok.

Harvey

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Harvey Harrison <harvey.harrison@gmail.com> wrote:

> > Al's patch is:
> > 
> > +                       if (op == '&' && expr->left->type == EXPR_PREOP &&
> > +                           expr->left->op == '!')
> > +                               warning(expr->pos, "dubious: !x & y");
> > 
> > i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
> > 
> 
> Well, (!x & y) and (!x | y) are probably the two that might have been 
> intended otherwise.  (x & !y), (x | !y) are probably ok.

i think the proper intention in the latter cases is (x & ~y) and
(x | ~y).

My strong bet is that in 99% of the cases they are real bugs and && or 
|| was intended.

	Ingo

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Derek M Jones]

All,

>>> i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
>>>
>> Well, (!x & y) and (!x | y) are probably the two that might have been 
>> intended otherwise.  (x & !y), (x | !y) are probably ok.
> 
> i think the proper intention in the latter cases is (x & ~y) and
> (x | ~y).
> 
> My strong bet is that in 99% of the cases they are real bugs and && or 
> || was intended.

Developer knowledge of operator precedence and the issue of what
they intended to write are interesting topics.  Some experimental
work is described in (binary operators only I'm afraid):

www.knosof.co.uk/cbook/accu06a.pdf
www.knosof.co.uk/cbook/accu07a.pdf

The ACCU 2006 experiment provides evidence that developer knowledge
is proportional to the number of occurrences of a construct in
source code, it also shows a stunningly high percentage of incorrect
answers.

The ACCU 2007 experiment provides evidence that the names of the
operands has a significant impact on operator precedence choice.

I wonder what kind of names are used as the operand of unary
operators?

I would expect the ~ operator to have a bitwise name, but the
! operator might have an arithmetic or bitwise name.

-- 
Derek M. Jones                              tel: +44 (0) 1252 520 667
Knowledge Software Ltd                      mailto:derek-Qjv84pu2YCLQXOPxS62xeg@public.gmane.org
Applications Standards Conformance Testing    http://www.knosof.co.uk
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Julia Lawall]

There are some legitimate uses of !x & y which are actually of the form !x 
& !y, where x and y are function calls.  That is a not particularly 
elegant way of getting both x and y to be evaluated and then combining the 
results using "and".  If such code is considered acceptable, then perhaps 
the sparse patch should be more complicated.

julia

> Al's patch is:
> 
> +                       if (op == '&' && expr->left->type == EXPR_PREOP &&
> +                           expr->left->op == '!')
> +                               warning(expr->pos, "dubious: !x & y");
> 
> i think there might be similar patterns: "x & !y", "!x | y", "x | !y" ?
> 
> 	Ingo
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Julia Lawall <julia@diku.dk> wrote:

> There are some legitimate uses of !x & y which are actually of the 
> form !x & !y, where x and y are function calls.  That is a not 
> particularly elegant way of getting both x and y to be evaluated and 
> then combining the results using "and".  If such code is considered 
> acceptable, then perhaps the sparse patch should be more complicated.

i tend to be of the opinion that the details in C source code should be 
visually obvious and should be heavily simplified down from what is 
'possible' language-wise - with most deviations and complications that 
depart from convention considered an error. I'd consider "!fn1() & 
!fn2()" a borderline coding style violation in any case - and it costs 
nothing to change it to "!fn1() && !fn2()".

	Ingo

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Bart Van Assche]

On Wed, Mar 5, 2008 at 1:20 PM, Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org> wrote:
>
>  * Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org> wrote:
>
>  > There are some legitimate uses of !x & y which are actually of the
>  > form !x & !y, where x and y are function calls.  That is a not
>  > particularly elegant way of getting both x and y to be evaluated and
>  > then combining the results using "and".  If such code is considered
>  > acceptable, then perhaps the sparse patch should be more complicated.
>
>  i tend to be of the opinion that the details in C source code should be
>  visually obvious and should be heavily simplified down from what is
>  'possible' language-wise - with most deviations and complications that
>  depart from convention considered an error. I'd consider "!fn1() &
>  !fn2()" a borderline coding style violation in any case - and it costs
>  nothing to change it to "!fn1() && !fn2()".

If someone writes (!x & !y) instead of (!x && !y) because both x and y
have to be evaluated, this means that both x and y have side effects.
Please keep in mind that the C language does not specify whether x or
y has to be evaluated first, so if x and y have to be evaluated in
that order, an expression like (!x & !y) can be the cause of very
subtle bugs. I prefer readability above brevity.

Bart Van Assche.
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Ingo Molnar]

* Bart Van Assche <bart.vanassche-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> If someone writes (!x & !y) instead of (!x && !y) because both x and y 
> have to be evaluated, this means that both x and y have side effects.
> Please keep in mind that the C language does not specify whether x or 
> y has to be evaluated first, so if x and y have to be evaluated in 
> that order, an expression like (!x & !y) can be the cause of very 
> subtle bugs. I prefer readability above brevity.

such expressions _must_ be written as:

  ret1 = x();
  ret2 = y();

  if (ret1 && ret2)
	...

any side-effects are totally un-obvious when they are in expressions and 
someone doing cleanups later on could easily change the '&' to '&&' and 
introduce a bug.

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/9] drivers/net/wireless/iwlwifi/iwl-4965.c: Correct use of ! and &

[Julia Lawall]

On Wed, 5 Mar 2008, Ingo Molnar wrote:

>
> * Julia Lawall <julia-dAYI7NvHqcQ@public.gmane.org> wrote:
>
> > There are some legitimate uses of !x & y which are actually of the
> > form !x & !y, where x and y are function calls.  That is a not
> > particularly elegant way of getting both x and y to be evaluated and
> > then combining the results using "and".  If such code is considered
> > acceptable, then perhaps the sparse patch should be more complicated.
>
> i tend to be of the opinion that the details in C source code should be
> visually obvious and should be heavily simplified down from what is
> 'possible' language-wise - with most deviations and complications that
> depart from convention considered an error. I'd consider "!fn1() &
> !fn2()" a borderline coding style violation in any case - and it costs
> nothing to change it to "!fn1() && !fn2()".

!fn1() && !fn2() does not have the same semantics as !fn1() & !fn2().  In
!fn1() & !fn2() both function calls are evaluated.  In !fn1() && !fn2(),
if !fn1() returns false then !fn2() is not evaluated.  I haven't studied
the particular instances of fn2(), though, to know whether it makes a
difference.

One could instead do something like:

x = fn1();
y = fn2();
if (!x && !y) ...

It would certainly be clearer, but more verbose.

julia

--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html