[PATCH] checkpatch: Make --strict a default for files in drivers/net and net/

[PATCH] checkpatch: Make --strict a default for files in drivers/net and net/

[Joe Perches]

Networking files are generally more strictly conformant to
linux-kernel style so make checkpatch more verbose by default for
patches to files or when checking files in these directories.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 85fc923..633e9f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -24,6 +24,7 @@ my $emacs = 0;
 my $terse = 0;
 my $file = 0;
 my $check = 0;
+my $check_orig = 0;
 my $summary = 1;
 my $mailback = 0;
 my $summary_file = 0;
@@ -146,6 +147,7 @@ GetOptions(
 help(0) if ($help);
 
 $fix = 1 if ($fix_inplace);
+$check_orig = $check;
 
 my $exit = 0;
 
@@ -1813,11 +1815,13 @@ sub process {
 		$here = "#$linenr: " if (!$file);
 		$here = "#$realline: " if ($file);
 
+		my $found_file = 0;
 		# extract the filename as it passes
 		if ($line =~ /^diff --git.*?(\S+)$/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
 			$in_commit_log = 0;
+			$found_file = 1;
 		} elsif ($line =~ /^\+\+\+\s+(\S+)/) {
 			$realfile = $1;
 			$realfile =~ s@^([^/]*)/@@ if (!$file);
@@ -1834,6 +1838,15 @@ sub process {
 				ERROR("MODIFIED_INCLUDE_ASM",
 				      "do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
 			}
+			$found_file = 1;
+		}
+
+		if ($found_file) {
+			if ($realfile =~ m@^(drivers/net/|net/)@) {
+				$check = 1;
+			} else {
+				$check = $check_orig;
+			}
 			next;
 		}
 
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH] checkpatch: Make --strict a default for files in drivers/net and net/

[Kim Phillips]

On Wed, 7 May 2014 11:13:26 -0700
Joe Perches <joe@perches.com> wrote:

> Networking files are generally more strictly conformant to
> linux-kernel style

checkpatch disagrees :) :

{drivers/}net/ :            ~10.8 CHECKs per .[hc] file
everything else:            ~10   CHECKs per .[hc] file
no net, no drivers/staging:  ~8.6 CHECKs per .[hc] file

(see [1] below for details).

> +		if ($found_file) {
> +			if ($realfile =~ m@^(drivers/net/|net/)@) {

this isn't easily extensible/scalable to other subsystems, or
say something like "all Freescale drivers."  Having it configurable
in .checkpatch.conf might be a better solution, but I don't believe
networking should be the only subsystem that can take advantage of
the extra checkpatch CHECKs.

Can we enable --strict universally in the Linux kernel, maybe like
so:?

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 34eb216..60e1a39 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -228,6 +228,7 @@ if ($tree) {
                print "Must be run from the top-level dir. of a kernel tree\n";
                exit(2);
        }
+       $check = 1;
 }
 
 my $emitted_corrupt = 0;

fwiw, --strict has been set unconditionally in u-boot's
.checkpatch.conf for over a year now, and has significantly reduced
patch revision churn for simple mistakes like parenthesis alignment,
spaces after a cast and/or before a semicolon...

Thanks,

Kim

[1] the following were run on kernel v3.15-rc4-260-g38583f0:

for i in `git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
328329
git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep \\\.[ch]$ | wc -l
32754
=> 32754 / 328329 ~= 10 CHECKs per file.[ch] for everything outside of {drivers/}net/.

for i in `git ls-files net/ drivers/net/ | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
40431
git ls-files net/ drivers/net/ | grep \\\.[ch]$ | wc -l
3742
=> {drivers/}net/: 40431 / 3742 = 10.8 CHECKs per file.[hc]

for i in `git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep -v '^drivers/staging/' | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
262005
git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep -v '^drivers/staging/' | grep \\\.[ch]$ | wc -l
30479
=> no net, no staging:  262005 / 30479  =  8.6 CHECKs per file.[hc]

Re: [PATCH] checkpatch: Make --strict a default for files in drivers/net and net/

[Joe Perches]

On Fri, 2014-05-09 at 15:37 -0500, Kim Phillips wrote:
> On Wed, 7 May 2014 11:13:26 -0700
> Joe Perches <joe@perches.com> wrote:
> 
> > Networking files are generally more strictly conformant to
> > linux-kernel style
> 
> checkpatch disagrees :) :
> 
> {drivers/}net/ :            ~10.8 CHECKs per .[hc] file
> everything else:            ~10   CHECKs per .[hc] file
> no net, no drivers/staging:  ~8.6 CHECKs per .[hc] file
> 
> (see [1] below for details).
> 
> > +		if ($found_file) {
> > +			if ($realfile =~ m@^(drivers/net/|net/)@) {
> 
> this isn't easily extensible/scalable to other subsystems, or
> say something like "all Freescale drivers."  Having it configurable
> in .checkpatch.conf might be a better solution, but I don't believe
> networking should be the only subsystem that can take advantage of
> the extra checkpatch CHECKs.

staging probably could too.

> Can we enable --strict universally in the Linux kernel, maybe like
> so:?

I don't think that's appropriate (yet?).

Anyone that wants --strict checking can either add
it on the command line or create a .checkpatch.conf
file with --strict in it.

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -228,6 +228,7 @@ if ($tree) {
>                 print "Must be run from the top-level dir. of a kernel tree\n";
>                 exit(2);
>         }
> +       $check = 1;
>  }
>  
>  my $emitted_corrupt = 0;
> 
> fwiw, --strict has been set unconditionally in u-boot's
> .checkpatch.conf for over a year now, and has significantly reduced
> patch revision churn for simple mistakes like parenthesis alignment,
> spaces after a cast and/or before a semicolon...

Good to know.

> [1] the following were run on kernel v3.15-rc4-260-g38583f0:
> 
> for i in `git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
> 328329
> git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep \\\.[ch]$ | wc -l
> 32754
> => 32754 / 328329 ~= 10 CHECKs per file.[ch] for everything outside of {drivers/}net/.
> 
> for i in `git ls-files net/ drivers/net/ | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
> 40431
> git ls-files net/ drivers/net/ | grep \\\.[ch]$ | wc -l
> 3742
> => {drivers/}net/: 40431 / 3742 = 10.8 CHECKs per file.[hc]
> 
> for i in `git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep -v '^drivers/staging/' | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
> 262005
> git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep -v '^drivers/staging/' | grep \\\.[ch]$ | wc -l
> 30479
> => no net, no staging:  262005 / 30479  =  8.6 CHECKs per file.[hc]

Try that with a per-file "wc -l" for checks per LOC

Re: [PATCH] checkpatch: Make --strict a default for files in drivers/net and net/

[Kim Phillips]

On Fri, 9 May 2014 13:56:15 -0700
Joe Perches <joe@perches.com> wrote:

> On Fri, 2014-05-09 at 15:37 -0500, Kim Phillips wrote:
> > On Wed, 7 May 2014 11:13:26 -0700
> > Joe Perches <joe@perches.com> wrote:
> > 
> > > Networking files are generally more strictly conformant to
> > > linux-kernel style
> > 
> > checkpatch disagrees :) :
> > 
> > {drivers/}net/ :            ~10.8 CHECKs per .[hc] file
> > everything else:            ~10   CHECKs per .[hc] file
> > no net, no drivers/staging:  ~8.6 CHECKs per .[hc] file
> > 
> > (see [1] below for details).
> > 
> > > +		if ($found_file) {
> > > +			if ($realfile =~ m@^(drivers/net/|net/)@) {
> > 
> > this isn't easily extensible/scalable to other subsystems, or
> > say something like "all Freescale drivers."  Having it configurable
> > in .checkpatch.conf might be a better solution, but I don't believe
> > networking should be the only subsystem that can take advantage of
> > the extra checkpatch CHECKs.
> 
> staging probably could too.

why call staging out to be subject to stricter checks than the rest
of the kernel, when it's code that's _known_ to be non-compliant?

> > Can we enable --strict universally in the Linux kernel, maybe like
> > so:?
> 
> I don't think that's appropriate (yet?).

why not?  checkpatch CHECKs seem reasonably stable AFAICT.

> Anyone that wants --strict checking can either add
> it on the command line

most aren't aware of --strict, and even if they are, easily forget
to add it.

> or create a .checkpatch.conf
> file with --strict in it.

right, but since checkpatch already has internal knowledge of the
linux kernel, I thought it inappropriate to add a .checkpatch.conf,
and use the $tree switch.

> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -228,6 +228,7 @@ if ($tree) {
> >                 print "Must be run from the top-level dir. of a kernel tree\n";
> >                 exit(2);
> >         }
> > +       $check = 1;
> >  }
> >  
> >  my $emitted_corrupt = 0;
> > 
> > fwiw, --strict has been set unconditionally in u-boot's
> > .checkpatch.conf for over a year now, and has significantly reduced
> > patch revision churn for simple mistakes like parenthesis alignment,
> > spaces after a cast and/or before a semicolon...
> 
> Good to know.

would an alternative option be to promote CHECKs like parentheses
alignment to WARN status?

> > [1] the following were run on kernel v3.15-rc4-260-g38583f0:
> > 
> > for i in `git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
> > 328329
> > git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep \\\.[ch]$ | wc -l
> > 32754
> > => 32754 / 328329 ~= 10 CHECKs per file.[ch] for everything outside of {drivers/}net/.
> > 
> > for i in `git ls-files net/ drivers/net/ | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
> > 40431
> > git ls-files net/ drivers/net/ | grep \\\.[ch]$ | wc -l
> > 3742
> > => {drivers/}net/: 40431 / 3742 = 10.8 CHECKs per file.[hc]
> > 
> > for i in `git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep -v '^drivers/staging/' | grep \\\.[ch]$`; do scripts/checkpatch.pl --strict -f $i; done | grep ^CHECK: | wc -l
> > 262005
> > git ls-files | grep -v '^net/' | grep -v '^drivers/net/' | grep -v '^drivers/staging/' | grep \\\.[ch]$ | wc -l
> > 30479
> > => no net, no staging:  262005 / 30479  =  8.6 CHECKs per file.[hc]
> 
> Try that with a per-file "wc -l" for checks per LOC

good point, that reinforces the statement back for networking :) :

{drivers/}net/ :            0.013 CHECKs per .[hc] file line
everything else:            0.43  CHECKs per .[hc] file line
no net, no drivers/staging: 0.38  CHECKs per .[hc] file line

Thanks,

Kim

Re: [PATCH] checkpatch: Make --strict a default for files in drivers/net and net/

[Joe Perches]

On Fri, 2014-05-09 at 16:29 -0500, Kim Phillips wrote:
[]
> > > this isn't easily extensible/scalable to other subsystems, or
> > > say something like "all Freescale drivers."  Having it configurable
> > > in .checkpatch.conf might be a better solution, but I don't believe
> > > networking should be the only subsystem that can take advantage of
> > > the extra checkpatch CHECKs.
> > 
> > staging probably could too.
> 
> why call staging out to be subject to stricter checks than the rest
> of the kernel, when it's code that's _known_ to be non-compliant?

Staging is mostly a "play area sandbox" for people
that want to submit "my first kernel patch" and
GregKH seems to apply style-only patches like that.

> > > Can we enable --strict universally in the Linux kernel, maybe like
> > > so:?
> > 
> > I don't think that's appropriate (yet?).
> 
> why not?  checkpatch CHECKs seem reasonably stable AFAICT.

Several maintainers have their own style or don't
care at all about what indentation is used.

Several maintainers prefer stasis to what they
perceive as changes that are just whitespace noise.

> would an alternative option be to promote CHECKs like parentheses
> alignment to WARN status?

Probably not.

> > Try that with a per-file "wc -l" for checks per LOC
> 
> good point, that reinforces the statement back for networking :) :
> 
> {drivers/}net/ :            0.013 CHECKs per .[hc] file line
> everything else:            0.43  CHECKs per .[hc] file line
> no net, no drivers/staging: 0.38  CHECKs per .[hc] file line

yup. cheers, Joe