2007-08-01 09:56:21

by Richard Knutsson

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

Mike Frysinger wrote:
> On 7/27/07, Robin Getz <[email protected]> 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)



2007-08-01 10:03:26

by Mike Frysinger

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

On 7/31/07, Richard Knutsson <[email protected]> wrote:
> Mike Frysinger wrote:
> > On 7/27/07, Robin Getz <[email protected]> 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

2007-08-01 10:38:44

by Richard Knutsson

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

Mike Frysinger wrote:
> On 7/31/07, Richard Knutsson <[email protected]> wrote:
>
>> Mike Frysinger wrote:
>>
>>> On 7/27/07, Robin Getz <[email protected]> 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

2007-08-01 20:28:30

by Matt Mackall

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

On Wed, Aug 01, 2007 at 12:38:39PM +0200, Richard Knutsson wrote:
> Mike Frysinger wrote:
> >On 7/31/07, Richard Knutsson <[email protected]> wrote:
> >
> >>Mike Frysinger wrote:
> >>
> >>>On 7/27/07, Robin Getz <[email protected]> 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.

2007-08-02 14:55:30

by Robin Getz

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

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