: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

PATCH In review Sergey Senozhatsky 5 participants 9 messages Apr 30, 2009 – May 2, 2009

Subsystems (2)

SCSI SCSI tape driver

lore

Share & tools

lore

Sergey Senozhatsky

author

2009-04-30 18:49 UTC #0 lore raw

PATCH 1/1 : fixing gcc 4.4 compiler warning: suggest parentheses around operand of ‘!’

Trivial: fixing gcc 4.4 compiler warning:
drivers/scsi/st.c: In function ‘st_int_ioctl’:
drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’

Signed-off-by Sergey Senozhatsky <[email protected]>

---

drivers/scsi/st.c +1 -1

index eb24efe..2bde8a5 100644
@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (!STp->use_pf) | PF_TESTED;
 				st_release_request(SRpnt);
 				SRpnt = NULL;
 				return st_int_ioctl(STp, cmd_in, arg);

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2009-04-30 21:31 UTC #1 lore raw

Quoted 13 lines · Sergey Senozhatsky

Trivial: fixing gcc 4.4 compiler warning:
drivers/scsi/st.c: In function ‘st_int_ioctl’:
drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
Signed-off-by Sergey Senozhatsky <[email protected]>
---
drivers/scsi/st.c +0 -0
index eb24efe..2bde8a5 100644

drivers/scsi/st.c static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon · line 2967

Quoted 5 lines

drivers/scsi/st.c +1 -1

@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (!STp->use_pf) | PF_TESTED;

This just doesn't look right. As I said to the warning reporter, I think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to check this.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andrew Morton

2009-04-30 21:49 UTC #2 lore raw

Quoted 9 lines · On Thu, 30 Apr 2009 21:31:24 +0000 James Bottomley…

Quoted 8 lines · Sergey Senozhatsky
Trivial: fixing gcc 4.4 compiler warning:
drivers/scsi/st.c: In function ‘st_int_ioctl’:
drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
Signed-off-by Sergey Senozhatsky <[email protected]>
---

Quoted 14 lines

drivers/scsi/st.c static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon · line 2967
Quoted 10 lines
drivers/scsi/st.c +1 -1
index eb24efe..2bde8a5 100644
@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (!STp->use_pf) | PF_TESTED;
This just doesn't look right. As I said to the warning reporter, I think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to check this.

yeah, good point. The code looks very confused.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Sergey Senozhatsky

author

2009-04-30 22:00 UTC #3 lore raw

In reply to James Bottomley

Quoted 32 lines · James Bottomley

To: Sergey Senozhatsky <[email protected]>
Cc: Andrew Morton <[email protected]>, [email protected],
Kai Makisara <[email protected]>
From: James Bottomley <[email protected]>
Subject: Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses
around operand of ‘!’
Quoted 13 lines · Sergey Senozhatsky
Trivial: fixing gcc 4.4 compiler warning:
drivers/scsi/st.c: In function ‘st_int_ioctl’:
drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
Signed-off-by Sergey Senozhatsky <[email protected]>
---
drivers/scsi/st.c +0 -0
index eb24efe..2bde8a5 100644
drivers/scsi/st.c static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon · line 2967
Quoted 5 lines
drivers/scsi/st.c +1 -1
@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (!STp->use_pf) | PF_TESTED;
This just doesn't look right. As I said to the warning reporter, I think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to check this.

Quoted 2 lines

James

Hello James.
It's not clear to me why STp->use_pf |= PF_TESTED; is correct fix
(since STp->use_pf = (!STp->use_pf) | PF_TESTED; is not equals to STp->use_pf |= PF_TESTED;).

Would you provide some explanations?

Thanks.

Sergey.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2009-04-30 22:08 UTC #4 lore raw

Quoted 33 lines · Sergey Senozhatsky

Quoted 32 lines · James Bottomley
To: Sergey Senozhatsky <[email protected]>
Cc: Andrew Morton <[email protected]>, [email protected],
Kai Makisara <[email protected]>
From: James Bottomley <[email protected]>
Subject: Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses
around operand of ‘!’
Quoted 13 lines · Sergey Senozhatsky
Trivial: fixing gcc 4.4 compiler warning:
drivers/scsi/st.c: In function ‘st_int_ioctl’:
drivers/scsi/st.c:2967: warning: suggest parentheses around operand of ‘!’ or change ‘|’ to ‘||’ or ‘!’ to ‘~’
Signed-off-by Sergey Senozhatsky <[email protected]>
---
drivers/scsi/st.c +0 -0
index eb24efe..2bde8a5 100644
drivers/scsi/st.c static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon · line 2967
Quoted 5 lines
drivers/scsi/st.c +1 -1
@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (!STp->use_pf) | PF_TESTED;
This just doesn't look right. As I said to the warning reporter, I think STp->use_pf |= PF_TESTED; is the correct fix, but I need Kai to check this.

Quoted 8 lines

Quoted 2 lines
James
Hello James.
It's not clear to me why STp->use_pf |= PF_TESTED; is correct fix
(since STp->use_pf = (!STp->use_pf) | PF_TESTED; is not equals to STp->use_pf |= PF_TESTED;).
Would you provide some explanations?

Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an incredibly obscure fashon. The flag PF_TESTED seems to imply that it's a don't check this again type deal, and the if clause is looking for it not being set, hence I think the most likely thing to be done here is set the PF_TESTED flag.

James

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Al Viro

2009-04-30 22:26 UTC #5 lore raw

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ???!???

Quoted 5 lines · James Bottomley

Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an incredibly obscure fashon. The flag PF_TESTED seems to imply that it's a don't check this again type deal, and the if clause is looking for it not being set, hence I think the most likely thing to be done here is set the PF_TESTED flag.

Not _set_ USE_PF; what's happening there is
^= USE_PF | PF_TESTED; /* we know that PF_TESTED was not set */

Sergey Senozhatsky

author

2009-04-30 23:10 UTC #6 lore raw

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ???!???

Quoted 11 lines · Al Viro

Quoted 5 lines · James Bottomley
Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an incredibly obscure fashon. The flag PF_TESTED seems to imply that it's a don't check this again type deal, and the if clause is looking for it not being set, hence I think the most likely thing to be done here is set the PF_TESTED flag.
Not _set_ USE_PF; what's happening there is
^= USE_PF | PF_TESTED; /* we know that PF_TESTED was not set */

st.h:#define PF_TESTED 2
st.h:#define USE_PF 1

st.c:2963
if (cmdstatp->sense_hdr.sense_key == ILLEGAL_REQUEST && !(STp->use_pf & PF_TESTED))

'&& !(STp->use_pf & PF_TESTED)' means: STp->use_pf == 0 or STp->use_pf != ... 0010 (PF_TESTED).

if == 0
STp->use_pf = !0 | 2 -> 3

if > PF_TESTED
STp->use_pf = 0 | PF_TESTED -> PF_TESTED

I need to think about

Quoted 1 line

I think the most likely thing to be done here is set the PF_TESTED flag.

Sergey.

Kai Makisara

2009-05-01 05:17 UTC #7 lore raw

Re: [PATCH]: fixing gcc 4.4 compiler warning: suggest parentheses around operand of ???!???

In reply to Al Viro

Quoted 11 lines · Al Viro

Quoted 5 lines · James Bottomley
Well, I know why (!STp->use_pf) is wrong: it would set USE_PF in an incredibly obscure fashon. The flag PF_TESTED seems to imply that it's a don't check this again type deal, and the if clause is looking for it not being set, hence I think the most likely thing to be done here is set the PF_TESTED flag.
Not _set_ USE_PF; what's happening there is
^= USE_PF | PF_TESTED; /* we know that PF_TESTED was not set */

Yes, this is the correct interpretation. This "bug" has been on my list of things to fix since David Binderman reported it to me earlier. The fix has waited because the current code is correct but difficult to read. The fix I have been planning is

        STp->use_pf = (STp->use_pf ^ USE_PF) | PF_TESTED;

This is a little more explicit in showing that we want to flip the bit masked by USE_PF and tell that we have tried both alternatives.

I can make a patch later today or tomorrow.

Thanks,
Kai

Kai Makisara

2009-05-02 05:49 UTC #8 lore raw

PATCH 1/1 SCSI tape: fix gcc 4.4 warning

This patch fixes the GCC 4.4 warning reported by David Binderman and Sergey Senozhatsky. The old version was working correctly but was not easy to read.

Signed-off-by Kai Makisara <[email protected]>

---
 drivers/scsi/st.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/st.c +1 -1

index eb24efe..c3334ef 100644
@@ -2964,7 +2964,7 @@ static int st_int_ioctl(struct scsi_tape *STp, unsigned int cmd_in, unsigned lon
 			    !(STp->use_pf & PF_TESTED)) {
 				/* Try the other possible state of Page Format if not
 				   already tried */
-				STp->use_pf = !STp->use_pf | PF_TESTED;
+				STp->use_pf = (STp->use_pf ^ USE_PF) | PF_TESTED;
 				st_release_request(SRpnt);
 				SRpnt = NULL;
 				return st_int_ioctl(STp, cmd_in, arg);