Re: if (a & X || b & ~Y) in dasd.c

Re: if (a & X || b & ~Y) in dasd.c

[Carsten Otte]

Hi Pete, Hi Kevin,

it's just like Kevin explained, the singe &/|/~ are bit operations
while &&/|| are logical operations. The bit operations are handled
before the logical ones (am I correct?)- so compiler-wise this should
be fine.
Sure, one should use some paranthesises to make things clearer,
but why are you examining  such ancient code? 2.2.14. seems a little
bit historical for me. The current dasd device driver is much more
readable and better designed (is redesigned in the 2.4 series).

mit freundlichem Gruß / with kind regards
Carsten Otte

IBM Deutschland Entwicklung GmbH
Linux for 390/zSeries Development - Device Driver Team
Phone: +49/07031/16-4076
IBM internal phone: *120-4076
--
We are Linux.
Resistance indicates that you're missing the point!


Kevin <kevin@pheared.net> on 11/08/2001 10:10:51 PM

Please respond to Kevin <kevin@pheared.net>

To:   Pete Zaitcev <zaitcev@redhat.com>
cc:   Carsten Otte/Germany/IBM@IBMDE, <linux-kernel@vger.kernel.org>,
      BOEBLINGEN LINUX390/Germany/IBM@IBMDE
Subject:  Re: if (a & X || b & ~Y) in dasd.c



On Thu, 8 Nov 2001, Pete Zaitcev wrote:

[zaitce] Date: Thu, 8 Nov 2001 15:57:49 -0500
[zaitce] From: Pete Zaitcev <zaitcev@redhat.com>
[zaitce] To: Cotte@de.ibm.com
[zaitce] Cc: linux-kernel@vger.kernel.org, Linux390@de.ibm.com
[zaitce] Subject: if (a & X || b & ~Y) in dasd.c
[zaitce]
[zaitce] Carsten and others:
[zaitce]
[zaitce] this code in 2.2.14 looks suspicious to me:
[zaitce]
[zaitce] ./drivers/s390/block/dasd.c:
[zaitce]         /* first of all lets try to find out the appropriate
era_action */
[zaitce]         if (stat->flag & DEVSTAT_FLAG_SENSE_AVAIL ||
[zaitce]             stat->dstat & ~(DEV_STAT_CHN_END | DEV_STAT_DEV_END))
{
[zaitce]                 /* anything abnormal ? */
[zaitce]                 if (device->discipline->examine_error == NULL ||
[zaitce]                     stat->flag & DEVSTAT_HALT_FUNCTION) {
[zaitce]                         era = dasd_era_fatal;
[zaitce]                 } else {
[zaitce]                         era = device->discipline->examine_error
(cqr, stat);
[zaitce]                 }
[zaitce]                 DASD_DRIVER_DEBUG_EVENT (1, dasd_int_handler,"
era_code %d",
[zaitce]                                          era);
[zaitce]         }
[zaitce]
[zaitce] Are you sure any parenthesises are not needed here?

I'm not going to pretend to know what the code is accomplishing, but as
a matter of C, those &'s aren't the same as &&'s and will get executed as
the || is tested.  So the first one will get &'ded and if false then the
second will and if neither return true then we move on.

Or, perhaps I'm being an idiot and misunderstanding the question and the
code.  In that case, I'll pretend I didn't say anything.  :)

-[ kevin@pheared.net                 devel.pheared.net ]-
-[ Rather be forgotten, than remembered for giving in. ]-
-[ ZZ = g ^ (xb * xa) mod p      g = h^{(p-1)/q} mod p ]-

if (a & X || b & ~Y) in dasd.c

[Pete Zaitcev]

Carsten and others:

this code in 2.2.14 looks suspicious to me:

./drivers/s390/block/dasd.c:
        /* first of all lets try to find out the appropriate era_action */
        if (stat->flag & DEVSTAT_FLAG_SENSE_AVAIL ||
            stat->dstat & ~(DEV_STAT_CHN_END | DEV_STAT_DEV_END)) {
                /* anything abnormal ? */
                if (device->discipline->examine_error == NULL ||
                    stat->flag & DEVSTAT_HALT_FUNCTION) {
                        era = dasd_era_fatal;
                } else {
                        era = device->discipline->examine_error (cqr, stat);
                }
                DASD_DRIVER_DEBUG_EVENT (1, dasd_int_handler," era_code %d",
                                         era);
        }

Are you sure any parenthesises are not needed here?

-- Pete

Re: if (a & X || b & ~Y) in dasd.c

[Kevin]

On Thu, 8 Nov 2001, Pete Zaitcev wrote:

[zaitce] Date: Thu, 8 Nov 2001 15:57:49 -0500
[zaitce] From: Pete Zaitcev <zaitcev@redhat.com>
[zaitce] To: Cotte@de.ibm.com
[zaitce] Cc: linux-kernel@vger.kernel.org, Linux390@de.ibm.com
[zaitce] Subject: if (a & X || b & ~Y) in dasd.c
[zaitce]
[zaitce] Carsten and others:
[zaitce]
[zaitce] this code in 2.2.14 looks suspicious to me:
[zaitce]
[zaitce] ./drivers/s390/block/dasd.c:
[zaitce]         /* first of all lets try to find out the appropriate era_action */
[zaitce]         if (stat->flag & DEVSTAT_FLAG_SENSE_AVAIL ||
[zaitce]             stat->dstat & ~(DEV_STAT_CHN_END | DEV_STAT_DEV_END)) {
[zaitce]                 /* anything abnormal ? */
[zaitce]                 if (device->discipline->examine_error == NULL ||
[zaitce]                     stat->flag & DEVSTAT_HALT_FUNCTION) {
[zaitce]                         era = dasd_era_fatal;
[zaitce]                 } else {
[zaitce]                         era = device->discipline->examine_error (cqr, stat);
[zaitce]                 }
[zaitce]                 DASD_DRIVER_DEBUG_EVENT (1, dasd_int_handler," era_code %d",
[zaitce]                                          era);
[zaitce]         }
[zaitce]
[zaitce] Are you sure any parenthesises are not needed here?

I'm not going to pretend to know what the code is accomplishing, but as
a matter of C, those &'s aren't the same as &&'s and will get executed as
the || is tested.  So the first one will get &'ded and if false then the
second will and if neither return true then we move on.

Or, perhaps I'm being an idiot and misunderstanding the question and the
code.  In that case, I'll pretend I didn't say anything.  :)

-[ kevin@pheared.net                 devel.pheared.net ]-
-[ Rather be forgotten, than remembered for giving in. ]-
-[ ZZ = g ^ (xb * xa) mod p      g = h^{(p-1)/q} mod p ]-

Re: if (a & X || b & ~Y) in dasd.c

[Jonathan Lundell]

At 3:57 PM -0500 11/8/01, Pete Zaitcev wrote:
>Carsten and others:
>
>this code in 2.2.14 looks suspicious to me:
>
>./drivers/s390/block/dasd.c:
>         /* first of all lets try to find out the appropriate era_action */
>         if (stat->flag & DEVSTAT_FLAG_SENSE_AVAIL ||
>             stat->dstat & ~(DEV_STAT_CHN_END | DEV_STAT_DEV_END)) {
>                 /* anything abnormal ? */
>                 if (device->discipline->examine_error == NULL ||
>                     stat->flag & DEVSTAT_HALT_FUNCTION) {
>                         era = dasd_era_fatal;
>                 } else {
>                         era = device->discipline->examine_error (cqr, stat);
>                 }
>                 DASD_DRIVER_DEBUG_EVENT (1, dasd_int_handler," era_code %d",
>                                          era);
>         }
>
>Are you sure any parenthesises are not needed here?

It's OK. You're probably used to seeing (necessary) parens when || is 
replaced by == in this kind of expression.
-- 
/Jonathan Lundell.

Re: if (a & X || b & ~Y) in dasd.c

[Richard B. Johnson]

On Thu, 8 Nov 2001, Pete Zaitcev wrote:

It looks okay, but it sure is hard to read. I will simplify...

> Carsten and others:
> 
> this code in 2.2.14 looks suspicious to me:
> 
> ./drivers/s390/block/dasd.c:
>         /* first of all lets try to find out the appropriate era_action */
>         if (stat->flag & DEVSTAT_FLAG_SENSE_AVAIL ||

          if (value & MASK ||

>             stat->dstat & ~(DEV_STAT_CHN_END | DEV_STAT_DEV_END)) {

              another_value & ANOTHER_MASK )

		(if ether of these are TRUE)

... etc.
 Don't confuse & with &&....

> Are you sure any parenthesises are not needed here?

No, but they sure would help to make the code readable by humans
as well as compilers.


Cheers,
Dick Johnson

Penguin : Linux version 2.4.1 on an i686 machine (799.53 BogoMips).

    I was going to compile a list of innovations that could be
    attributed to Microsoft. Once I realized that Ctrl-Alt-Del
    was handled in the BIOS, I found that there aren't any.

Re: if (a & X || b & ~Y) in dasd.c

[bill davidsen]

In article <20011108155749.A24023@devserv.devel.redhat.com> zaitcev@redhat.com wrote:
>Carsten and others:
>
>this code in 2.2.14 looks suspicious to me:
>
>./drivers/s390/block/dasd.c:
>        /* first of all lets try to find out the appropriate era_action */
>        if (stat->flag & DEVSTAT_FLAG_SENSE_AVAIL ||
>            stat->dstat & ~(DEV_STAT_CHN_END | DEV_STAT_DEV_END)) {

  If the code does what I think it does, it works as written. However, I
usually would throw in parenthesis on something like this to be sure
that the next person reading the code won't waste time thinking about
it. I always thought that good code was literature, which could be read,
understood, and enjoyed by many.

-- 
bill davidsen <davidsen@tmr.com>
  His first management concern is not solving the problem, but covering
his ass. If he lived in the middle ages he'd wear his codpiece backward.

Re: if (a & X || b & ~Y) in dasd.c

[Peter T. Breuer]

"bill davidsen wrote:"
>   If the code does what I think it does, it works as written. However, I
> usually would throw in parenthesis on something like this to be sure
> that the next person reading the code won't waste time thinking about

Which is WHY you do not put in parentheses.

C is designed with precedences that make sense. You can write
conditions the way they ought to be written, without parentheses.

   a && b || c && d

is read exactly the same way as you would read

   a * b  +  c * d

and I have no idea why people would think otherwise. && is the logical
multiplicative operator (1*x = x, x*0 = 0, x*y = y*x, x*(y*z) = (x*y)*z),
and || is the logical addition operator (x+0 = x, x+y = y +x,
x+(y+z)=(x+y)+z)) and they distribute correctly (x*(y+z) = (x*y)+(x*z)),
so why should you treat them as though they were some strange thing
from mars that needs parentheses?

> it. I always thought that good code was literature, which could be read,
> understood, and enjoyed by many.

Exactly so. So don't put in extra punctuation to help people that can't
read or you'll spoil it for us. Parentheses make things illegible, as
surely as ((2)=(((1))+((1)))) :-). If an expression is difficult for
you, don't write it!

Additive operators in C have weaker priority than multiplicative
operators at the same level.  = is weaker than anything.  Relational
operators are next weakest.  Then bitwise operators.  Then logical
operators.  Then arithmetic operators, then monary operators, which are
strongest of all.

That's the natural way. It means you can write, instead of

   *x++ += (((a+2) << 2) > (b-4));

   *x++ += a+2 << 2  >  b-4;

(but that's illegible simply by virtue of having too complicated a RHS
in either case), or

   x = a & b || c & d;

because the & binds more tightly than || by virtue of being
bitwise, not logical. If you were to write

   x = a & b | c & d;

that  would also be right, because & binds more tightly by virtue of
being multiplicative.

   x = a && b || c && d

is also right, for the same reason. All have the same logical
semantics, two have the same bitwise semantics. Two have the
same arithmetic semantics, I think ... obviously you can also add

   x = a * b + c * d

to the list, and more variants - though you wouldn't want to.

Where you might get caught out is by not realizing that << is 
weaker than arithmetic ops, so 

   *x++ += a+2 << 2  >  b-4;

(what I wrote before) is NOT

   *x++ += a + 2<<2  >  b-4;

nor

   *x++ += a+2  <<  2 > b-4

nor

   *x++ += a   +   2  <<  2>b   -   4

Blech. I can't think of any more eccentric interpretations. The
principle is that the higher level and more additive an operator is,
the weaker it is.  This is intuitive. You only need to parenthise
weak operators when you want them to be evaluated before a strong
operator acts. Thus

  *x++ +=  *((x | 0x01) + (1<<*x)) 

to be silly. The commonest mistake is writing

  x = y << z + w

instead of

  x = y << (z++w)

But compare

  x = y ** z + w

(should the power operator exist) and you'll see why it's wrong. << is
precisely a power operator, and not treating it as such is the error.


Peter

Re: if (a & X || b & ~Y) in dasd.c

[Stevie O]

At 08:38 PM 11/19/2001 +0100, Peter T. Breuer wrote:

>"bill davidsen wrote:"
> >   If the code does what I think it does, it works as written. However, I
> > usually would throw in parenthesis on something like this to be sure
> > that the next person reading the code won't waste time thinking about
>
>Which is WHY you do not put in parentheses.

<snip snip>

So instead of using extra parentheses, we should include a copy of your 
response in every potentially ambiguous location instead?

Two C constructs that have bitten me in the glutinous maximus on more than 
one occasion:


mem_address = mem_base + page_index << 12;      // Wrong!

if ( bit_mask & BIT_FLAG == 0) { flag_not_set(); }      // Wrong!

Of course C has precedence. It's not always obvious. And the difference 
between this

x = y + z << 2;

and

x = (y + z) << 2;

is that the 2nd doesn't make me have to remember the relative precedences 
of + and <<.


--
Stevie-O

REAL kernel hackers use
# cat > /vmlinuz
and
# insmod /dev/stdin