[PATCH 06/68] 0 -> NULL, for arch/frv

[PATCH 06/68] 0 -> NULL, for arch/frv

[Yoann Padioleau]

When comparing a pointer, it's clearer to compare it to NULL than to 0.

Here is an excerpt of the semantic patch: 

@@
expression *E;
@@

  E ==
- 0
+ NULL

@@
expression *E;
@@

  E !=
- 0
+ NULL

Signed-off-by: Yoann Padioleau <padator@wanadoo.fr>
Cc: dhowells@redhat.com
Cc: akpm@linux-foundation.org
---

 dma-alloc.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/frv/mm/dma-alloc.c b/arch/frv/mm/dma-alloc.c
index dc6522c..eb67cef 100644
--- a/arch/frv/mm/dma-alloc.c
+++ b/arch/frv/mm/dma-alloc.c
@@ -61,7 +61,7 @@ static int map_page(unsigned long va, un
 
 	/* Use middle 10 bits of VA to index the second-level map */
 	pte = pte_alloc_kernel(pme, va);
-	if (pte != 0) {
+	if (pte != NULL) {
 		err = 0;
 		set_pte(pte, mk_pte_phys(pa & PAGE_MASK, prot));
 	}
@@ -99,7 +99,7 @@ void *consistent_alloc(gfp_t gfp, size_t
 
 	/* allocate some common virtual space to map the new pages */
 	area = get_vm_area(size, VM_ALLOC);
-	if (area == 0) {
+	if (area == NULL) {
 		free_pages(page, order);
 		return NULL;
 	}

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Al Viro]

On Fri, Jul 27, 2007 at 11:44:35AM +0200, Yoann Padioleau wrote:
>  	pte = pte_alloc_kernel(pme, va);
> -	if (pte != 0) {
> +	if (pte != NULL) {
>  		err = 0;
>  		set_pte(pte, mk_pte_phys(pa & PAGE_MASK, prot));
>  	}
> @@ -99,7 +99,7 @@ void *consistent_alloc(gfp_t gfp, size_t
>  
>  	/* allocate some common virtual space to map the new pages */
>  	area = get_vm_area(size, VM_ALLOC);
> -	if (area == 0) {
> +	if (area == NULL) {
>  		free_pages(page, order);
>  		return NULL;

Same comment about comparisons with NULL after allocation...

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[David Howells]

Yoann Padioleau <padator@wanadoo.fr> wrote:

> When comparing a pointer, it's clearer to compare it to NULL than to 0.

Can you make them of style:

	if (!x)

instead?

Thanks,
David

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Yoann Padioleau]

David Howells <dhowells@redhat.com> writes:

> Yoann Padioleau <padator@wanadoo.fr> wrote:
>
>> When comparing a pointer, it's clearer to compare it to NULL than to 0.
>
> Can you make them of style:
>
> 	if (!x)

Yes I can. I can make another semantic patch later to do that
transformation. But some people may prefer (x == NULL) to (!x)
so I don't know. I think that transformation 
some 0 to NULL is less controversial.


>
> instead?
>
> Thanks,
> David

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Yoann Padioleau]

Al Viro <viro@ftp.linux.org.uk> writes:

> On Fri, Jul 27, 2007 at 11:44:35AM +0200, Yoann Padioleau wrote:
>>  	pte = pte_alloc_kernel(pme, va);
>> -	if (pte != 0) {
>> +	if (pte != NULL) {
>>  		err = 0;
>>  		set_pte(pte, mk_pte_phys(pa & PAGE_MASK, prot));
>>  	}
>> @@ -99,7 +99,7 @@ void *consistent_alloc(gfp_t gfp, size_t
>>  
>>  	/* allocate some common virtual space to map the new pages */
>>  	area = get_vm_area(size, VM_ALLOC);
>> -	if (area == 0) {
>> +	if (area == NULL) {
>>  		free_pages(page, order);
>>  		return NULL;
>
> Same comment about comparisons with NULL after allocation...

I don't understand. pte is a pointer right ? So why should we
keep the == 0 ? 

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Al Viro]

On Fri, Jul 27, 2007 at 12:21:53PM +0200, Yoann Padioleau wrote:
> > On Fri, Jul 27, 2007 at 11:44:35AM +0200, Yoann Padioleau wrote:
> >>  	pte = pte_alloc_kernel(pme, va);
> >> -	if (pte != 0) {
> >> +	if (pte != NULL) {
 
> I don't understand. pte is a pointer right ? So why should we
> keep the == 0 ? 

Idiomatic form for "has allocation succeeded?" is neither "if (p != 0)" nor
"if (p != NULL)".  It's simply "if (p)".

Note that it depends upon context.  For something that combines assignment
with test
	if ((p = foo_alloc()) != NULL)
would be the right way to go.  Ditto for
	flag = (p == NULL)
(alternative would be "flag = !p", which is usually not nice or even
"flag = !!p" for the opposite test, and that's bloody atrocious).

For places like
-      if (spu_disassemble_table[o] == 0)
+      if (spu_disassemble_table[o] == NULL)
        spu_disassemble_table[o] = &spu_opcodes[i];
it's a matter of taste; there I'd go for explicit comparison with NULL.
I'd also go for explicit comparisons in places like
-               wait_event(journal->j_wait_done_commit, journal->j_task == 0);
+               wait_event(journal->j_wait_done_commit, journal->j_task == NULL);

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Mike Frysinger]

On 7/27/07, Robin Getz <rgetz@blackfin.uclinux.org> wrote:
> If there is a definite style or semantic preference that everyone should live
> with - does it make sense to put checks in checkpatch.pl to enforce it?

checkpatch.pl does not have enough semantic knowledge to know if the
thing being tested is a pointer ... dont know if the sparse utility
would be able to pick it out as i'm not familiar with what level that
thing runs at
-mike

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Robin Getz]

On Fri 27 Jul 2007 06:18, Yoann Padioleau pondered:
> David Howells <dhowells@redhat.com> writes:
> 
> > Yoann Padioleau <padator@wanadoo.fr> wrote:
> >
> >> When comparing a pointer, it's clearer to compare it to NULL than to
> 0.
> >
> > Can you make them of style:
> >
> > 	if (!x)
> 
> Yes I can. I can make another semantic patch later to do that
> transformation. But some people may prefer (x == NULL) to (!x)
> so I don't know. I think that transformation 
> some 0 to NULL is less controversial.
> 
> 
> >
> > instead?

If there is a definite style or semantic preference that everyone should live 
with - does it make sense to put checks in checkpatch.pl to enforce it?

-Robin

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Richard Knutsson]

Mike Frysinger wrote:
> On 7/27/07, Robin Getz <rgetz@blackfin.uclinux.org> wrote:
>   
>> If there is a definite style or semantic preference that everyone should live
>> with - does it make sense to put checks in checkpatch.pl to enforce it?
>>     
>
> checkpatch.pl does not have enough semantic knowledge to know if the
> thing being tested is a pointer ... dont know if the sparse utility
> would be able to pick it out as i'm not familiar with what level that
> thing runs at
>   
Didn't he mean "x == NULL" > "!x"?

Richard Knutsson
(wrote this offline so if someone has already reply, then sorry about 
the noise)

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Mike Frysinger]

On 7/31/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
> Mike Frysinger wrote:
> > On 7/27/07, Robin Getz <rgetz@blackfin.uclinux.org> wrote:
> >> If there is a definite style or semantic preference that everyone should live
> >> with - does it make sense to put checks in checkpatch.pl to enforce it?
> >
> > checkpatch.pl does not have enough semantic knowledge to know if the
> > thing being tested is a pointer ... dont know if the sparse utility
> > would be able to pick it out as i'm not familiar with what level that
> > thing runs at
>
> Didn't he mean "x == NULL" > "!x"?

i'm sure i understand your meaning of ">" ... are you saying that "x
== NULL" is greater (preferred) to "!x" or are you saying that "x ==
NULL" should be changed to "!x" ?

i dont think the former case can be checked by checkpatch.pl, but the
latter certainly can ... but i'd be very skeptical you could get the
wider LKML audience to sign off one way or the other wrt to "x ==
NULL" vs "!x".  you can certainly get people to sign off on "x == 0"
being wrong when x is a pointer.
-mike

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Richard Knutsson]

Mike Frysinger wrote:
> On 7/31/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
>   
>> Mike Frysinger wrote:
>>     
>>> On 7/27/07, Robin Getz <rgetz@blackfin.uclinux.org> wrote:
>>>       
>>>> If there is a definite style or semantic preference that everyone should live
>>>> with - does it make sense to put checks in checkpatch.pl to enforce it?
>>>>         
>>> checkpatch.pl does not have enough semantic knowledge to know if the
>>> thing being tested is a pointer ... dont know if the sparse utility
>>> would be able to pick it out as i'm not familiar with what level that
>>> thing runs at
>>>       
>> Didn't he mean "x == NULL" > "!x"?
>>     
>
> i'm sure i understand your meaning of ">" ... are you saying that "x
> == NULL" is greater (preferred) to "!x" or are you saying that "x ==
> NULL" should be changed to "!x" ?
>   
If I understood Robin correctly, he suggested that checkpatch.pl would 
tell to convert "x == NULL" to "!x", if that would be the preferred way.
> i dont think the former case can be checked by checkpatch.pl, but the
> latter certainly can ... but i'd be very skeptical you could get the
> wider LKML audience to sign off one way or the other wrt to "x ==
> NULL" vs "!x".  you can certainly get people to sign off on "x == 0"
> being wrong when x is a pointer.
>   
I agree!
BTW, too bad checkpatch.pl does not know the types, since it otherwise 
could check for the "x [=!]= 0"-thing.

Richard Knutsson

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Matt Mackall]

On Wed, Aug 01, 2007 at 12:38:39PM +0200, Richard Knutsson wrote:
> Mike Frysinger wrote:
> >On 7/31/07, Richard Knutsson <ricknu-0@student.ltu.se> wrote:
> >  
> >>Mike Frysinger wrote:
> >>    
> >>>On 7/27/07, Robin Getz <rgetz@blackfin.uclinux.org> wrote:
> >>>      
> >>>>If there is a definite style or semantic preference that everyone 
> >>>>should live
> >>>>with - does it make sense to put checks in checkpatch.pl to enforce it?
> >>>>        
> >>>checkpatch.pl does not have enough semantic knowledge to know if the
> >>>thing being tested is a pointer ... dont know if the sparse utility
> >>>would be able to pick it out as i'm not familiar with what level that
> >>>thing runs at
> >>>      
> >>Didn't he mean "x == NULL" > "!x"?
> >>    
> >
> >i'm sure i understand your meaning of ">" ... are you saying that "x
> >== NULL" is greater (preferred) to "!x" or are you saying that "x ==
> >NULL" should be changed to "!x" ?
> >  
> If I understood Robin correctly, he suggested that checkpatch.pl would 
> tell to convert "x == NULL" to "!x", if that would be the preferred way.
> >i dont think the former case can be checked by checkpatch.pl, but the
> >latter certainly can ... but i'd be very skeptical you could get the
> >wider LKML audience to sign off one way or the other wrt to "x ==
> >NULL" vs "!x".  you can certainly get people to sign off on "x == 0"
> >being wrong when x is a pointer.
> >  
> I agree!
> BTW, too bad checkpatch.pl does not know the types, since it otherwise 
> could check for the "x [=!]= 0"-thing.

About the only place that if (x != 0) is preferred to if (x) is cases
where the 0 value doesn't semantically correspond to false/off/disabled.
And that's basically thing that return 0 for success and negative errors.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH 06/68] 0 -> NULL, for arch/frv

[Robin Getz]

On Wed 1 Aug 2007 06:38, Richard Knutsson pondered:
> If I understood Robin correctly, he suggested that checkpatch.pl would 
> tell to convert "x == NULL" to "!x", if that would be the preferred way.

I guess I was asking - _if_ this is really important - lets pick a 
preferred way, and try to use the existing infrastructure (sparce 
or checkpatch.pl) to try to enforce it.

If it is just pedantic - let it go. There are lots of ways to do the 
same thing, one is not necessarily more correct. (but in this case - 
I see the point, and agree).

> BTW, too bad checkpatch.pl does not know the types, since it otherwise 
> could check for the "x [=!]= 0"-thing.

Then let's ask people to use sparse on anything in their diffstat.
(or at least recommend it).

Sparse isn't mentioned in Documentation/SubmittingPatches yet. (if
it is a suggestion)

Even something completely stupid like:

diff -uprN old new | diffstat -p 0 | grep -v changed | awk '{print $1}' | sed 's/\.c/\.o/' | xargs make C=2

worked for me.

-Robin