Re: update checkpatch.pl to version 0.19

Re: update checkpatch.pl to version 0.19

[Greg KH]

On Fri, Jun 06, 2008 at 07:22:25PM +0000, Linux Kernel Mailing List wrote:
> @@ -1920,23 +2004,16 @@ sub process {
>  				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
>  			}
>  		}
> -# check for needless usb_free_urb() checks
> -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> -			my $expr = $1;
> -			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
> -				WARN("usb_free_urb(NULL) is safe this check is probabally not required\n" . $hereprev);
> -			}
> -		}
>  

I'm curious as to why this check was removed.  Any specific reason?  It
was valid from what I can tell.

thanks,

greg k-h

Re: update checkpatch.pl to version 0.19

[Andrew Morton]

On Fri, 6 Jun 2008 16:30:37 -0700 Greg KH <greg@kroah.com> wrote:

> On Fri, Jun 06, 2008 at 07:22:25PM +0000, Linux Kernel Mailing List wrote:
> > @@ -1920,23 +2004,16 @@ sub process {
> >  				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
> >  			}
> >  		}
> > -# check for needless usb_free_urb() checks
> > -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> > -			my $expr = $1;
> > -			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
> > -				WARN("usb_free_urb(NULL) is safe this check is probabally not required\n" . $hereprev);
> > -			}
> > -		}
> >  
> 
> I'm curious as to why this check was removed.  Any specific reason?  It
> was valid from what I can tell.

Because of the spelling?

If so, let's be consistent:

--- a/scripts/checkpatch.pl~a
+++ a/scripts/checkpatch.pl
@@ -1997,14 +1997,6 @@ sub process {
 				$herecurr);
 		}
 
-# check for needless kfree() checks
-		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
-			my $expr = $1;
-			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
-				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
-			}
-		}
-
 # warn about #ifdefs in C files
 #		if ($line =~ /^.\s*\#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
 #			print "#ifdef in C files should be avoided\n";
_

Re: update checkpatch.pl to version 0.19

[Greg KH]

On Fri, Jun 06, 2008 at 11:02:39PM -0700, Andrew Morton wrote:
> On Fri, 6 Jun 2008 16:30:37 -0700 Greg KH <greg@kroah.com> wrote:
> 
> > On Fri, Jun 06, 2008 at 07:22:25PM +0000, Linux Kernel Mailing List wrote:
> > > @@ -1920,23 +2004,16 @@ sub process {
> > >  				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
> > >  			}
> > >  		}
> > > -# check for needless usb_free_urb() checks
> > > -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> > > -			my $expr = $1;
> > > -			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
> > > -				WARN("usb_free_urb(NULL) is safe this check is probabally not required\n" . $hereprev);
> > > -			}
> > > -		}
> > >  
> > 
> > I'm curious as to why this check was removed.  Any specific reason?  It
> > was valid from what I can tell.
> 
> Because of the spelling?
> 
> If so, let's be consistent:
> 
> --- a/scripts/checkpatch.pl~a
> +++ a/scripts/checkpatch.pl
> @@ -1997,14 +1997,6 @@ sub process {
>  				$herecurr);
>  		}
>  
> -# check for needless kfree() checks
> -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> -			my $expr = $1;
> -			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
> -				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
> -			}
> -		}
> -

That's the proverbial baby with the bathwater problem here, we can fix
spelling mistakes pretty easily :)

I'd prefer the original check to be put back, and the kfree(NULL) check
to remain as well, as it too is valid.

But I would like to find out first from Andy why this was removed.  Odds
are he wasn't keeping up with the changes upstream from his local copy,
which also might have caused other things to be removed over time :(

thanks,

greg k-h

Re: update checkpatch.pl to version 0.19

[Andy Whitcroft]

On Sat, Jun 07, 2008 at 10:34:36AM -0700, Greg KH wrote:
> On Fri, Jun 06, 2008 at 11:02:39PM -0700, Andrew Morton wrote:
> > On Fri, 6 Jun 2008 16:30:37 -0700 Greg KH <greg@kroah.com> wrote:
> > 
> > > On Fri, Jun 06, 2008 at 07:22:25PM +0000, Linux Kernel Mailing List wrote:
> > > > @@ -1920,23 +2004,16 @@ sub process {
> > > >  				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
> > > >  			}
> > > >  		}
> > > > -# check for needless usb_free_urb() checks
> > > > -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> > > > -			my $expr = $1;
> > > > -			if ($line =~ /\busb_free_urb\(\Q$expr\E\);/) {
> > > > -				WARN("usb_free_urb(NULL) is safe this check is probabally not required\n" . $hereprev);
> > > > -			}
> > > > -		}
> > > >  
> > > 
> > > I'm curious as to why this check was removed.  Any specific reason?  It
> > > was valid from what I can tell.
> > 
> > Because of the spelling?
> > 
> > If so, let's be consistent:
> > 
> > --- a/scripts/checkpatch.pl~a
> > +++ a/scripts/checkpatch.pl
> > @@ -1997,14 +1997,6 @@ sub process {
> >  				$herecurr);
> >  		}
> >  
> > -# check for needless kfree() checks
> > -		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
> > -			my $expr = $1;
> > -			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
> > -				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
> > -			}
> > -		}
> > -
> 
> That's the proverbial baby with the bathwater problem here, we can fix
> spelling mistakes pretty easily :)
> 
> I'd prefer the original check to be put back, and the kfree(NULL) check
> to remain as well, as it too is valid.
> 
> But I would like to find out first from Andy why this was removed.  Odds
> are he wasn't keeping up with the changes upstream from his local copy,
> which also might have caused other things to be removed over time :(

That look to be my fault.  Lost something that got merged by some other
route.  I really need a workable solution to the tests problem so I can
work with real per change deltas.

I do try and check things are not removed erroneously.  Seems I did a
poor job on that one tho.  The delta was pretty heafty as I remember, so
I missed it.

The issue I have is I have a comprehensive test suite which is kept in
lock step with checkpatch itself, all in my own tree.  Now that is just
noise for mainline.  What I want to be able to do is keep that in lock
step with developments in my tree, but not merge it upstream.  So far
I've not found a workable solution with git for that.

Anyhow I will sort out where that check has gone and reinstate it.
Perhaps for the next release I will move to pushing Andrew as a set of
patches, not a delta that would stop this occuring again.

-apw

Re: update checkpatch.pl to version 0.19

[David Woodhouse]

On Mon, 2008-06-09 at 09:33 +0100, Andy Whitcroft wrote:
> The issue I have is I have a comprehensive test suite which is kept in
> lock step with checkpatch itself, all in my own tree.  Now that is
> just noise for mainline.  What I want to be able to do is keep that in
> lock step with developments in my tree, but not merge it upstream.  So
> far I've not found a workable solution with git for that.

Why not make a tree which automatically tracks Linus' tree but only
includes checkpatch.pl. Then pull from that into your own tree before
you push your changes back.

-- 
dwmw2

Re: update checkpatch.pl to version 0.19

[Greg KH]

On Mon, Jun 09, 2008 at 12:09:41PM +0100, David Woodhouse wrote:
> On Mon, 2008-06-09 at 09:33 +0100, Andy Whitcroft wrote:
> > The issue I have is I have a comprehensive test suite which is kept in
> > lock step with checkpatch itself, all in my own tree.  Now that is
> > just noise for mainline.  What I want to be able to do is keep that in
> > lock step with developments in my tree, but not merge it upstream.  So
> > far I've not found a workable solution with git for that.
> 
> Why not make a tree which automatically tracks Linus' tree but only
> includes checkpatch.pl. Then pull from that into your own tree before
> you push your changes back.

Or just use quilt.  Or git.  Or something that takes into account that
your file will change in the mainline tree.

Keeping code outside the kernel tree and then blindly doing a diff to
generate a patch for submission is never considered a good idea, and
causes real problems, as seen here.

thanks,

greg k-h

Re: update checkpatch.pl to version 0.19

[Andy Whitcroft]

On Mon, Jun 09, 2008 at 03:58:43PM -0700, Greg KH wrote:
> On Mon, Jun 09, 2008 at 12:09:41PM +0100, David Woodhouse wrote:
> > On Mon, 2008-06-09 at 09:33 +0100, Andy Whitcroft wrote:
> > > The issue I have is I have a comprehensive test suite which is kept in
> > > lock step with checkpatch itself, all in my own tree.  Now that is
> > > just noise for mainline.  What I want to be able to do is keep that in
> > > lock step with developments in my tree, but not merge it upstream.  So
> > > far I've not found a workable solution with git for that.
> > 
> > Why not make a tree which automatically tracks Linus' tree but only
> > includes checkpatch.pl. Then pull from that into your own tree before
> > you push your changes back.
> 
> Or just use quilt.  Or git.  Or something that takes into account that
> your file will change in the mainline tree.
> 
> Keeping code outside the kernel tree and then blindly doing a diff to
> generate a patch for submission is never considered a good idea, and
> causes real problems, as seen here.

Indeed, and I am using git.  It is the fact that the surrounding test
suite is not submitted that means I need to work in a separate tree.  I
have since this incident looked back over the history and yours was the
only change that did not come through me and so the problem has been
restricted to that one change.

But to prevent it occuring again I have rejigged my tree so that I can
simply maintain a real git branch for just the checkpatch changes, in
parallel to the version with the tests.  This then allows me to merge
mainline and handle things correctly.

-apw

Re: update checkpatch.pl to version 0.19

[Roland Dreier]

 > The issue I have is I have a comprehensive test suite which is kept in
 > lock step with checkpatch itself, all in my own tree.  Now that is just
 > noise for mainline.  What I want to be able to do is keep that in lock
 > step with developments in my tree, but not merge it upstream.  So far
 > I've not found a workable solution with git for that.

>From my naive point of view: why wouldn't we want a test suite upstream,
if one is already written?

 - R.