Re: [PATCH] ppp: don't use 0 in pointer context
Paul Mackerras writes: And the solution is to treat it as a boolean instead?! I'm not sure which is more ugly. Why wouldn't explicit comparison against NULL be the preferred fix? I just think this whole you shouldn't compare a pointer to 0 thing is completely silly, Indeed. However, the head penguin seems to have some bee in his bonnet about 0 not being a pointer value (despite what the C standard says), so whatever. *shrug* Yeesh. My point was that if you're going to get bent over something silly like this, you might as well change it to something explicit, such as a comparison of two pointers, rather than pretending that either pointers or integers are in fact booleans. I realize that it's entirely legal (per the standards) to write: if (p) as it is to write: if (p != 0) and also: if (p != NULL) But if we're actually interested in clarity, rather than passing some woe-begotten lint-like purity test, only that third form actually says what the code does. The other two may well be idiomatic in certain circles, but they're nowhere near as lucid for those maintaining the code. I think that if this patch is important, then something other than just clarity is valued. In that case, just ignore me, and do what you will. -- James Carlson 42.703N 71.076W [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ppp: don't use 0 in pointer context
Signed-off-by: Alexey Dobriyan [EMAIL PROTECTED]
---
ppp is the biggest offender.
drivers/net/ppp_async.c | 34 ++--
drivers/net/ppp_generic.c | 128 +++---
drivers/net/ppp_synctty.c | 26 -
drivers/net/pppoe.c |2
4 files changed, 95 insertions(+), 95 deletions(-)
--- a/drivers/net/ppp_async.c
+++ b/drivers/net/ppp_async.c
@@ -159,7 +159,7 @@ ppp_asynctty_open(struct tty_struct *tty
err = -ENOMEM;
ap = kmalloc(sizeof(*ap), GFP_KERNEL);
- if (ap == 0)
+ if (!ap)
goto out;
/* initialize the asyncppp structure */
@@ -215,7 +215,7 @@ ppp_asynctty_close(struct tty_struct *tt
ap = tty-disc_data;
tty-disc_data = NULL;
write_unlock_irq(disc_data_lock);
- if (ap == 0)
+ if (!ap)
return;
/*
@@ -230,10 +230,10 @@ ppp_asynctty_close(struct tty_struct *tt
tasklet_kill(ap-tsk);
ppp_unregister_channel(ap-chan);
- if (ap-rpkt != 0)
+ if (ap-rpkt)
kfree_skb(ap-rpkt);
skb_queue_purge(ap-rqueue);
- if (ap-tpkt != 0)
+ if (ap-tpkt)
kfree_skb(ap-tpkt);
kfree(ap);
}
@@ -285,13 +285,13 @@ ppp_asynctty_ioctl(struct tty_struct *tt
int err, val;
int __user *p = (int __user *)arg;
- if (ap == 0)
+ if (!ap)
return -ENXIO;
err = -EFAULT;
switch (cmd) {
case PPPIOCGCHAN:
err = -ENXIO;
- if (ap == 0)
+ if (!ap)
break;
err = -EFAULT;
if (put_user(ppp_channel_index(ap-chan), p))
@@ -301,7 +301,7 @@ ppp_asynctty_ioctl(struct tty_struct *tt
case PPPIOCGUNIT:
err = -ENXIO;
- if (ap == 0)
+ if (!ap)
break;
err = -EFAULT;
if (put_user(ppp_unit_number(ap-chan), p))
@@ -354,7 +354,7 @@ ppp_asynctty_receive(struct tty_struct *
struct asyncppp *ap = ap_get(tty);
unsigned long flags;
- if (ap == 0)
+ if (!ap)
return;
spin_lock_irqsave(ap-recv_lock, flags);
ppp_async_input(ap, buf, cflags, count);
@@ -373,7 +373,7 @@ ppp_asynctty_wakeup(struct tty_struct *t
struct asyncppp *ap = ap_get(tty);
clear_bit(TTY_DO_WRITE_WAKEUP, tty-flags);
- if (ap == 0)
+ if (!ap)
return;
set_bit(XMIT_WAKEUP, ap-xmit_flags);
tasklet_schedule(ap-tsk);
@@ -688,7 +688,7 @@ ppp_async_push(struct asyncppp *ap)
tty_stuffed = 1;
continue;
}
- if (ap-optr = ap-olim ap-tpkt != 0) {
+ if (ap-optr = ap-olim ap-tpkt) {
if (ppp_async_encode(ap)) {
/* finished processing ap-tpkt */
clear_bit(XMIT_FULL, ap-xmit_flags);
@@ -708,7 +708,7 @@ ppp_async_push(struct asyncppp *ap)
clear_bit(XMIT_BUSY, ap-xmit_flags);
/* any more work to do? if not, exit the loop */
if (!(test_bit(XMIT_WAKEUP, ap-xmit_flags)
- || (!tty_stuffed ap-tpkt != 0)))
+ || (!tty_stuffed ap-tpkt)))
break;
/* more work to do, see if we can do it now */
if (test_and_set_bit(XMIT_BUSY, ap-xmit_flags))
@@ -719,7 +719,7 @@ ppp_async_push(struct asyncppp *ap)
flush:
clear_bit(XMIT_BUSY, ap-xmit_flags);
- if (ap-tpkt != 0) {
+ if (ap-tpkt) {
kfree_skb(ap-tpkt);
ap-tpkt = NULL;
clear_bit(XMIT_FULL, ap-xmit_flags);
@@ -852,7 +852,7 @@ ppp_async_input(struct asyncppp *ap, con
s = 0;
for (i = 0; i count; ++i) {
c = buf[i];
- if (flags != 0 flags[i] != 0)
+ if (flags flags[i] != 0)
continue;
s |= (c 0x80)? SC_RCV_B7_1: SC_RCV_B7_0;
c = ((c 4) ^ c) 0xf;
@@ -869,7 +869,7 @@ ppp_async_input(struct asyncppp *ap, con
n = scan_ordinary(ap, buf, count);
f = 0;
- if (flags != 0 (ap-state SC_TOSS) == 0) {
+ if (flags (ap-state SC_TOSS) == 0) {
/* check the flags to see if any char had an error */
for (j = 0; j n; ++j)
if ((f = flags[j]) != 0)
@@ -882,9 +882,9 @@ ppp_async_input(struct asyncppp *ap, con
} else if (n 0 (ap-state SC_TOSS) == 0) {
/* stuff the chars in the skb */
skb = ap-rpkt;
- if (skb == 0) {
+ if
Re: [PATCH] ppp: don't use 0 in pointer context
Alexey Dobriyan writes: - if (ap == 0) + if (!ap) And the solution is to treat it as a boolean instead?! I'm not sure which is more ugly. Why wouldn't explicit comparison against NULL be the preferred fix? -- James Carlson 42.703N 71.076W [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
James Carlson [EMAIL PROTECTED] wrote:
Alexey Dobriyan writes:
- if (ap == 0)
+ if (!ap)
And the solution is to treat it as a boolean instead?! I'm not sure
which is more ugly.
Treating it as a boolean looks good to me. It's better than the existing
code because it shuts sparse up.
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
James And the solution is to treat it as a boolean instead?! I'm James not sure which is more ugly. James Why wouldn't explicit comparison against NULL be the James preferred fix? if (ptr) and if (!ptr) are the preferred idiom for testing whether a pointer is NULL. What is gained by writing if (ptr != NULL) ? - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
[EMAIL PROTECTED] wrote on 02/08/2006 02:19:20 PM: James Carlson [EMAIL PROTECTED] wrote: Alexey Dobriyan writes: - if (ap == 0) + if (!ap) And the solution is to treat it as a boolean instead?! I'm not sure which is more ugly. Treating it as a boolean looks good to me. It's better than the existing code because it shuts sparse up. Why would sparse complain about this? 0 is a well-defined pointer value (the only value guaranteed to be by the language). From KR ANSI C edition, section 5.4: Pointers and integers are not interchangeable. Zero is the sole exception: the constant zero may be assigned to a pointer, and a pointer may be compared with the constant zero. +-DLS - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
From: David Stevens [EMAIL PROTECTED] Date: Wed, 8 Feb 2006 14:45:08 -0800 Why would sparse complain about this? 0 is a well-defined pointer value (the only value guaranteed to be by the language). Because sparse goes beyond the standards and tries to catch cases that usually end up being bugs. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
James Carlson writes: Alexey Dobriyan writes: - if (ap == 0) + if (!ap) And the solution is to treat it as a boolean instead?! I'm not sure which is more ugly. Why wouldn't explicit comparison against NULL be the preferred fix? I just think this whole you shouldn't compare a pointer to 0 thing is completely silly, and patches like this have about the same level of usefulness as patches that change color to colour or vice versa. However, the head penguin seems to have some bee in his bonnet about 0 not being a pointer value (despite what the C standard says), so whatever. *shrug* Paul. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
David S. Miller writes: Because sparse goes beyond the standards and tries to catch cases that usually end up being bugs. When has a pointer comparison with an explicit 0 ever caused a bug? Paul. - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
David S. Miller wrote: From: David Stevens [EMAIL PROTECTED] Date: Wed, 8 Feb 2006 14:45:08 -0800 Why would sparse complain about this? 0 is a well-defined pointer value (the only value guaranteed to be by the language). Because sparse goes beyond the standards and tries to catch cases that usually end up being bugs. So, how might it tell when someone is mistakenly using a pointer as a boolean if it takes a pass on if(foo) or if(!foo) instead of looking for if(foo == NULL) or if(foo != NULL)?(or I suppose if (NULL == foo) and if (NULL != foo)) it would seem that the compars with NULL, when read by pseudorandom folks would be more likely to reinforce that foo is a pointer rather than a boolean/flag. rick jones - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
On Wed, Feb 08, 2006 at 02:45:08PM -0800, David Stevens wrote:
[EMAIL PROTECTED] wrote on 02/08/2006 02:19:20 PM:
James Carlson [EMAIL PROTECTED] wrote:
Alexey Dobriyan writes:
- if (ap == 0)
+ if (!ap)
And the solution is to treat it as a boolean instead?! I'm not sure
which is more ugly.
Treating it as a boolean looks good to me. It's better than the
existing
code because it shuts sparse up.
Why would sparse complain about this? 0 is a well-defined
pointer value (the only value guaranteed to be by the language).
From KR ANSI C edition, section 5.4:
Pointers and integers are not interchangeable. Zero is the sole
exception: the constant zero may be assigned to a pointer, and a pointer
may be compared with the constant zero.
O is an integer, NULL is a pointer. You compare integers with integers
and pointers with pointers. You don't compare integers with pointers
because they are fundamentally different objects. The former counts
things, the latter points to them.
The fact that they can be represented by the same bit patterns is
irrelevant. The fact that KR, C89, C99 make
p = malloc();
if (p == NULL)
legal C is irrelevant.
Because
p = malloc();
if (!p)
err;
is idiomatic C, I've used this form.
full-disclosure
Oh, and for the record: current sparse from Linus doesn't warn about
this. Slightly modified sparse warns. Bugs which were uncovered by
more or less trivial and slightly broken sparse patch [1] are:
[PATCH] dscc4: fix dscc4_init_dummy_skb check
[PATCH] opl3sa2: fix adding second dma channel
[PATCH] gusclassic: fix adding second dma channel
[PATCH] ipw2200: fix -eeprom[EEPROM_VERSION] check
[PATCH] ixj: fix writing silence check
ppp-NULL.patch is the biggest part of
187 files changed, 494 insertions(+), 499 deletions(-)
so I've flushed it first.
[1]:
--- a/evaluate.c
+++ b/evaluate.c
@@ -934,6 +934,10 @@ static struct symbol *evaluate_compare(s
/* Pointer types? */
if (is_ptr_type(ltype) || is_ptr_type(rtype)) {
// FIXME! Check the types for compatibility
+ if (is_ptr_type(ltype) !is_ptr_type(rtype))
+ bad_expr_type(expr);
+ if (!is_ptr_type(ltype) is_ptr_type(rtype))
+ bad_expr_type(expr);
expr-op = modify_for_unsigned(expr-op);
goto OK;
}
/full-disclosure
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
On Wed, Feb 08, 2006 at 02:47:12PM -0800, David S. Miller wrote:
From: David Stevens [EMAIL PROTECTED]
Date: Wed, 8 Feb 2006 14:45:08 -0800
Why would sparse complain about this? 0 is a well-defined
pointer value (the only value guaranteed to be by the language).
Because sparse goes beyond the standards and tries to
catch cases that usually end up being bugs.
Well to be frank the only significant NULL/0 bug that I know can't
be caught by converting 0 to NULL anyway. The bug only exists on
architectures where the representations of null-pointers of types
are different. On those architectures, if you have something like
int func(char *fmt, ...);
...
func(foo, NULL);
Then this may break if func actually expected a int * while NULL
could be something different altogether. The only safe way to
write this is to have
func(foo, (int *)NULL);
or
func(foo, (int *)0);
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
Alexey Dobriyan [EMAIL PROTECTED] wrote:
Oh, and for the record: current sparse from Linus doesn't warn about
this. Slightly modified sparse warns. Bugs which were uncovered by
more or less trivial and slightly broken sparse patch [1] are:
[PATCH] dscc4: fix dscc4_init_dummy_skb check
[PATCH] opl3sa2: fix adding second dma channel
[PATCH] gusclassic: fix adding second dma channel
[PATCH] ipw2200: fix -eeprom[EEPROM_VERSION] check
[PATCH] ixj: fix writing silence check
The first two can also be caught with gcc -pedantic. In fact, catching
the last two should be doable there as well. The difference between
gcc -pedantic and sparse is that it doesn't warn about obviously correct
cases like p != 0 or p = 0.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
From: Herbert Xu [EMAIL PROTECTED] Date: Thu, 09 Feb 2006 10:49:37 +1100 The difference between gcc -pedantic and sparse is that it doesn't warn about obviously correct cases like p != 0 or p = 0. So obviously correct that you left out an equals sign in the second case :-) - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
On Wed, 8 Feb 2006, David S. Miller wrote: From: Herbert Xu [EMAIL PROTECTED] Date: Thu, 09 Feb 2006 10:49:37 +1100 The difference between gcc -pedantic and sparse is that it doesn't warn about obviously correct cases like p != 0 or p = 0. So obviously correct that you left out an equals sign in the second case :-) You should thank him. 8;) -- ~Randy - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ppp: don't use 0 in pointer context
On Wed, Feb 08, 2006 at 03:58:59PM -0800, David S. Miller wrote:
From: Herbert Xu [EMAIL PROTECTED]
Date: Thu, 09 Feb 2006 10:49:37 +1100
The difference between gcc -pedantic and sparse is that it doesn't
warn about obviously correct cases like p != 0 or p = 0.
So obviously correct that you left out an equals sign in the
second case :-)
Actually I intended that to be an assignment as this is something that
has generated many sparse patches. Now if this was a comparison
mistakenly written as an assignment, gcc would pick it up anyway:
a.c:3: warning: suggest parentheses around assignment used as truth value
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} [EMAIL PROTECTED]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
