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
On Thu, 8 Nov 2001, Pete Zaitcev wrote:
[zaitce] Date: Thu, 8 Nov 2001 15:57:49 -0500
[zaitce] From: Pete Zaitcev <[email protected]>
[zaitce] To: [email protected]
[zaitce] Cc: [email protected], [email protected]
[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. :)
-[ [email protected] devel.pheared.net ]-
-[ Rather be forgotten, than remembered for giving in. ]-
-[ ZZ = g ^ (xb * xa) mod p g = h^{(p-1)/q} mod p ]-
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.
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.
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 <[email protected]> on 11/08/2001 10:10:51 PM
Please respond to Kevin <[email protected]>
To: Pete Zaitcev <[email protected]>
cc: Carsten Otte/Germany/IBM@IBMDE, <[email protected]>,
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 <[email protected]>
[zaitce] To: [email protected]
[zaitce] Cc: [email protected], [email protected]
[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. :)
-[ [email protected] devel.pheared.net ]-
-[ Rather be forgotten, than remembered for giving in. ]-
-[ ZZ = g ^ (xb * xa) mod p g = h^{(p-1)/q} mod p ]-
In article <[email protected]> [email protected] 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 <[email protected]>
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.
"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
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