[PATCH 0/5] checkpatch: A few corrections and updates

[PATCH 0/5] checkpatch: A few corrections and updates

[Joe Perches]

Joe Perches (5):
  checkpatch: Add tests for function pointer style misuses
  checkpatch: Add a --fix-inplace option
  checkpatch: Improve space before tab --fix option
  checkpatch: check for if's with unnecessary parentheses
  checkpatch: Update the FSF/GPL address check

 scripts/checkpatch.pl | 100 ++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 8 deletions(-)

-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 1/5] checkpatch: Add tests for function pointer style misuses

[Joe Perches]

Kernel style uses function pointers in this form:
        "type (*funcptr)(args...)"

Emit warnings when this function pointer form isn't used.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 0fcbc5f..f5d4560 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2810,6 +2810,65 @@ sub process {
 			}
 		}
 
+# Function pointer declarations
+# check spacing between type, funcptr, and args
+# canonical declaration is "type (*funcptr)(args...)"
+#
+# the $Declare variable will capture all spaces after the type
+# so check it for multiple spaces
+		if ($line =~ /^.\s*($Declare)\((\s*)\*(\s*)$Ident(\s*)\)(\s*)\(/) {
+			my $declare = $1;
+			my $pre_pointer_space = $2;
+			my $post_pointer_space = $3;
+			my $funcname = $4;
+			my $post_funcname_space = $5;
+			my $pre_args_space = $6;
+
+			if ($declare !~ /\s$/) {
+				WARN("SPACING",
+				     "missing space after return type\n" . $herecurr);
+			}
+
+# unnecessary space "type  (*funcptr)(args...)"
+			elsif ($declare =~ /\s{2,}$/) {
+				WARN("SPACING",
+				     "Multiple spaces after return type\n" . $herecurr);
+			}
+
+# unnecessary space "type ( *funcptr)(args...)"
+			if (defined $pre_pointer_space &&
+			    $pre_pointer_space =~ /^\s/) {
+				WARN("SPACING",
+				     "Unnecessary space after function pointer open parenthesis\n" . $herecurr);
+			}
+
+# unnecessary space "type (* funcptr)(args...)"
+			if (defined $post_pointer_space &&
+			    $post_pointer_space =~ /^\s/) {
+				WARN("SPACING",
+				     "Unnecessary space before function pointer name\n" . $herecurr);
+			}
+
+# unnecessary space "type (*funcptr )(args...)"
+			if (defined $post_funcname_space &&
+			    $post_funcname_space =~ /^\s/) {
+				WARN("SPACING",
+				     "Unnecessary space after function pointer name\n" . $herecurr);
+			}
+
+# unnecessary space "type (*funcptr) (args...)"
+			if (defined $pre_args_space &&
+			    $pre_args_space =~ /^\s/) {
+				WARN("SPACING",
+				     "Unnecessary space before function pointer name\n" . $herecurr);
+			}
+
+			if (show_type("SPACING") && $fix) {
+				$fixed[$linenr - 1] =~
+				    s/^(.\s*$Declare)\(\s*\*\s*($Ident)\s*\)\s*\(/rtrim($1) . " " . "\(\*$2\)\("/ex;
+			}
+		}
+
 # check for spacing round square brackets; allowed:
 #  1. with a type on the left -- int [] a;
 #  2. at the beginning of a line for slice initialisers -- [0...10] = 5,
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 1/5] checkpatch: Add tests for function pointer style misuses

[Josh Triplett]

On Thu, Dec 26, 2013 at 11:20:28AM -0800, Joe Perches wrote:
> Kernel style uses function pointers in this form:
>         "type (*funcptr)(args...)"
> 
> Emit warnings when this function pointer form isn't used.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Two comments below.

>  scripts/checkpatch.pl | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 0fcbc5f..f5d4560 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2810,6 +2810,65 @@ sub process {
>  			}
>  		}
>  
> +# Function pointer declarations
> +# check spacing between type, funcptr, and args
> +# canonical declaration is "type (*funcptr)(args...)"
> +#
> +# the $Declare variable will capture all spaces after the type
> +# so check it for multiple spaces
> +		if ($line =~ /^.\s*($Declare)\((\s*)\*(\s*)$Ident(\s*)\)(\s*)\(/) {
> +			my $declare = $1;
> +			my $pre_pointer_space = $2;
> +			my $post_pointer_space = $3;
> +			my $funcname = $4;
> +			my $post_funcname_space = $5;
> +			my $pre_args_space = $6;
> +
> +			if ($declare !~ /\s$/) {
> +				WARN("SPACING",
> +				     "missing space after return type\n" . $herecurr);
> +			}
> +
> +# unnecessary space "type  (*funcptr)(args...)"
> +			elsif ($declare =~ /\s{2,}$/) {
> +				WARN("SPACING",
> +				     "Multiple spaces after return type\n" . $herecurr);
> +			}
> +
> +# unnecessary space "type ( *funcptr)(args...)"
> +			if (defined $pre_pointer_space &&
> +			    $pre_pointer_space =~ /^\s/) {
> +				WARN("SPACING",
> +				     "Unnecessary space after function pointer open parenthesis\n" . $herecurr);

There are two parentheticals in a proper function pointer declaration,
so this is slightly ambiguous.  Perhaps "between open parenthesis and
'*' in function pointer declaration"?

> +# unnecessary space "type (* funcptr)(args...)"
> +			if (defined $post_pointer_space &&
> +			    $post_pointer_space =~ /^\s/) {
> +				WARN("SPACING",
> +				     "Unnecessary space before function pointer name\n" . $herecurr);
> +			}
> +
> +# unnecessary space "type (*funcptr )(args...)"
> +			if (defined $post_funcname_space &&
> +			    $post_funcname_space =~ /^\s/) {
> +				WARN("SPACING",
> +				     "Unnecessary space after function pointer name\n" . $herecurr);
> +			}
> +
> +# unnecessary space "type (*funcptr) (args...)"
> +			if (defined $pre_args_space &&
> +			    $pre_args_space =~ /^\s/) {
> +				WARN("SPACING",
> +				     "Unnecessary space before function pointer name\n" . $herecurr);

Copy/paste problem here?  I think this should be "Unnecessary space
before function pointer argument types" or similar.

With both of the above two issues fixed:

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

Re: [PATCH 1/5] checkpatch: Add tests for function pointer style misuses

[Josh Triplett]

On Thu, Dec 26, 2013 at 12:07:23PM -0800, Josh Triplett wrote:
> On Thu, Dec 26, 2013 at 11:20:28AM -0800, Joe Perches wrote:
> > Kernel style uses function pointers in this form:
> >         "type (*funcptr)(args...)"
> > 
> > Emit warnings when this function pointer form isn't used.
> > 
> > Signed-off-by: Joe Perches <joe@perches.com>
> 
> Two comments below.

On Thu, Dec 26, 2013 at 02:16:10PM -0800, akpm@linux-foundation.org wrote:
> The patch titled
>      Subject: checkpatch: add tests for function pointer style misuses
> has been added to the -mm tree.  Its filename is
>      checkpatch-add-tests-for-function-pointer-style-misuses.patch

Here's a patch that fixes the two messages I commented about, which
applies on top of the version of the patch that went into -mm.

---8<---
From: Josh Triplett <josh@joshtriplett.org>
Subject: [PATCH] Fix up messages from "checkpatch: Add tests for function pointer style misuses"

Fix an ambiguity in one warning message and a copy/paste problem in
another.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2460094..b74b086 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2831,7 +2831,7 @@ sub process {
 			if (defined $pre_pointer_space &&
 			    $pre_pointer_space =~ /^\s/) {
 				WARN("SPACING",
-				     "Unnecessary space after function pointer open parenthesis\n" . $herecurr);
+				     "Unnecessary space between open parenthesis and '*' in function pointer declaration\n" . $herecurr);
 			}
 
 # unnecessary space "type (* funcptr)(args...)"
@@ -2852,7 +2852,7 @@ sub process {
 			if (defined $pre_args_space &&
 			    $pre_args_space =~ /^\s/) {
 				WARN("SPACING",
-				     "Unnecessary space before function pointer name\n" . $herecurr);
+				     "Unnecessary space before function pointer argument list\n" . $herecurr);
 			}
 
 			if (show_type("SPACING") && $fix) {
-- 
1.8.5.2

Re: [PATCH 1/5] checkpatch: Add tests for function pointer style misuses

[Joe Perches]

On Thu, 2013-12-26 at 16:58 -0800, Josh Triplett wrote:
> On Thu, Dec 26, 2013 at 12:07:23PM -0800, Josh Triplett wrote:
> > On Thu, Dec 26, 2013 at 11:20:28AM -0800, Joe Perches wrote:
> > > Kernel style uses function pointers in this form:
> > >         "type (*funcptr)(args...)"
> > > 
> > > Emit warnings when this function pointer form isn't used.
[]
> Here's a patch that fixes the two messages I commented about, which
> applies on top of the version of the patch that went into -mm.

Acked-by: Joe Perches <joe@perches.com>

thanks Josh, Joe

[PATCH 2/5] checkpatch: Add a --fix-inplace option

[Joe Perches]

Add the ability to fix and overwrite existing files/patches instead
of creating a new file "<filename>.EXPERIMENTAL-checkpatch-fixes".

Suggested-by: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5d4560..19376b4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -29,6 +29,7 @@ my $mailback = 0;
 my $summary_file = 0;
 my $show_types = 0;
 my $fix = 0;
+my $fix_inplace = 0;
 my $root;
 my %debug;
 my %camelcase = ();
@@ -76,6 +77,9 @@ Options:
                              "<inputfile>.EXPERIMENTAL-checkpatch-fixes"
                              with potential errors corrected to the preferred
                              checkpatch style
+  --fix-inplace              EXPERIMENTAL - may create horrible results
+                             Is the same as --fix, but overwrites the input
+                             file.  It's your fault if there's no backup or git
   --ignore-perl-version      override checking of perl version.  expect
                              runtime errors.
   -h, --help, --version      display this help and exit
@@ -131,6 +135,7 @@ GetOptions(
 	'mailback!'	=> \$mailback,
 	'summary-file!'	=> \$summary_file,
 	'fix!'		=> \$fix,
+	'fix-inplace!'	=> \$fix_inplace,
 	'ignore-perl-version!' => \$ignore_perl_version,
 	'debug=s'	=> \%debug,
 	'test-only=s'	=> \$tst_only,
@@ -140,6 +145,8 @@ GetOptions(
 
 help(0) if ($help);
 
+$fix = 1 if ($fix_inplace);
+
 my $exit = 0;
 
 if ($^V && $^V lt $minimum_perl_version) {
@@ -4456,7 +4463,8 @@ sub process {
 	hash_show_words(\%ignore_type, "Ignored");
 
 	if ($clean == 0 && $fix && "@rawlines" ne "@fixed") {
-		my $newfile = $filename . ".EXPERIMENTAL-checkpatch-fixes";
+		my $newfile = $filename;
+		$newfile .= ".EXPERIMENTAL-checkpatch-fixes" if (!$fix_inplace);
 		my $linecount = 0;
 		my $f;
 
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 2/5] checkpatch: Add a --fix-inplace option

[Josh Triplett]

On Thu, Dec 26, 2013 at 11:20:29AM -0800, Joe Perches wrote:
> Add the ability to fix and overwrite existing files/patches instead
> of creating a new file "<filename>.EXPERIMENTAL-checkpatch-fixes".
> 
> Suggested-by: Manfred Spraul <manfred@colorfullife.com>
> Signed-off-by: Joe Perches <joe@perches.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  scripts/checkpatch.pl | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index f5d4560..19376b4 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -29,6 +29,7 @@ my $mailback = 0;
>  my $summary_file = 0;
>  my $show_types = 0;
>  my $fix = 0;
> +my $fix_inplace = 0;
>  my $root;
>  my %debug;
>  my %camelcase = ();
> @@ -76,6 +77,9 @@ Options:
>                               "<inputfile>.EXPERIMENTAL-checkpatch-fixes"
>                               with potential errors corrected to the preferred
>                               checkpatch style
> +  --fix-inplace              EXPERIMENTAL - may create horrible results
> +                             Is the same as --fix, but overwrites the input
> +                             file.  It's your fault if there's no backup or git
>    --ignore-perl-version      override checking of perl version.  expect
>                               runtime errors.
>    -h, --help, --version      display this help and exit
> @@ -131,6 +135,7 @@ GetOptions(
>  	'mailback!'	=> \$mailback,
>  	'summary-file!'	=> \$summary_file,
>  	'fix!'		=> \$fix,
> +	'fix-inplace!'	=> \$fix_inplace,
>  	'ignore-perl-version!' => \$ignore_perl_version,
>  	'debug=s'	=> \%debug,
>  	'test-only=s'	=> \$tst_only,
> @@ -140,6 +145,8 @@ GetOptions(
>  
>  help(0) if ($help);
>  
> +$fix = 1 if ($fix_inplace);
> +
>  my $exit = 0;
>  
>  if ($^V && $^V lt $minimum_perl_version) {
> @@ -4456,7 +4463,8 @@ sub process {
>  	hash_show_words(\%ignore_type, "Ignored");
>  
>  	if ($clean == 0 && $fix && "@rawlines" ne "@fixed") {
> -		my $newfile = $filename . ".EXPERIMENTAL-checkpatch-fixes";
> +		my $newfile = $filename;
> +		$newfile .= ".EXPERIMENTAL-checkpatch-fixes" if (!$fix_inplace);
>  		my $linecount = 0;
>  		my $f;
>  
> -- 
> 1.8.1.2.459.gbcd45b4.dirty
> 

[PATCH 3/5] checkpatch: Improve space before tab --fix option

[Joe Perches]

This test should remove all the spaces before a tab not
just one space.

Substitute a tab for each 8 space block before a tab and
remove less than 8 spaces before a tab.

This SPACE_BEFORE_TAB test is done after CODE_INDENT.

If there are spaces used at the beginning of a line that
should be converted to tabs, please make sure that the
CODE_INDENT test and conversion is done before this
SPACE_BEFORE_TAB test and conversion.

Reported-by: Manfred Spraul <manfred@colorfullife.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 19376b4..db72b88 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2118,8 +2118,10 @@ sub process {
 			if (WARN("SPACE_BEFORE_TAB",
 				"please, no space before tabs\n" . $herevet) &&
 			    $fix) {
-				$fixed[$linenr - 1] =~
-				    s/(^\+.*) +\t/$1\t/;
+				while ($fixed[$linenr - 1] =~
+					   s/(^\+.*) {8,8}+\t/$1\t\t/) {}
+				while ($fixed[$linenr - 1] =~
+					   s/(^\+.*) +\t/$1\t/) {}
 			}
 		}
 
-- 
1.8.1.2.459.gbcd45b4.dirty

[PATCH 4/5] checkpatch: check for if's with unnecessary parentheses

[Joe Perches]

If statements don't need multiple parentheses around tested
comparisons like "if ((foo == bar))".

An == comparison maybe a sign of an intended assignment,
so emit a slightly different message if so.

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index db72b88..c12234e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3322,6 +3322,20 @@ sub process {
 			}
 		}
 
+# if statements using unnecessary parentheses - ie: if ((foo == bar))
+		if ($^V && $^V ge 5.10.0 &&
+		    $line =~ /\bif\s*((?:\(\s*){2,})/) {
+			my $openparens = $1;
+			my $count = $openparens =~ tr@\(@\(@;
+			my $msg = "";
+			if ($line =~ /\bif\s*(?:\(\s*){$count,$count}$LvalOrFunc\s*($Compare)\s*$LvalOrFunc(?:\s*\)){$count,$count}/) {
+				my $comp = $4;	#Not $1 because of $LvalOrFunc
+				$msg = " - maybe == should be = ?" if ($comp eq "==");
+				WARN("UNNECESSARY_PARENTHESES",
+				     "Unnecessary parentheses$msg\n" . $herecurr);
+			}
+		}
+
 # Return of what appears to be an errno should normally be -'ve
 		if ($line =~ /^.\s*return\s*(E[A-Z]*)\s*;/) {
 			my $name = $1;
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [PATCH 4/5] checkpatch: check for if's with unnecessary parentheses

[Josh Triplett]

On Thu, Dec 26, 2013 at 11:20:31AM -0800, Joe Perches wrote:
> If statements don't need multiple parentheses around tested
> comparisons like "if ((foo == bar))".
> 
> An == comparison maybe a sign of an intended assignment,
> so emit a slightly different message if so.
> 
> Signed-off-by: Joe Perches <joe@perches.com>

Reviewed-by: Josh Triplett <josh@joshtriplett.org>

>  scripts/checkpatch.pl | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index db72b88..c12234e 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3322,6 +3322,20 @@ sub process {
>  			}
>  		}
>  
> +# if statements using unnecessary parentheses - ie: if ((foo == bar))
> +		if ($^V && $^V ge 5.10.0 &&
> +		    $line =~ /\bif\s*((?:\(\s*){2,})/) {
> +			my $openparens = $1;
> +			my $count = $openparens =~ tr@\(@\(@;
> +			my $msg = "";
> +			if ($line =~ /\bif\s*(?:\(\s*){$count,$count}$LvalOrFunc\s*($Compare)\s*$LvalOrFunc(?:\s*\)){$count,$count}/) {
> +				my $comp = $4;	#Not $1 because of $LvalOrFunc
> +				$msg = " - maybe == should be = ?" if ($comp eq "==");
> +				WARN("UNNECESSARY_PARENTHESES",
> +				     "Unnecessary parentheses$msg\n" . $herecurr);
> +			}
> +		}
> +
>  # Return of what appears to be an errno should normally be -'ve
>  		if ($line =~ /^.\s*return\s*(E[A-Z]*)\s*;/) {
>  			my $name = $1;
> -- 
> 1.8.1.2.459.gbcd45b4.dirty
> 

[PATCH 5/5] checkpatch: Update the FSF/GPL address check

[Joe Perches]

The FSF address check is a bit too verbose looking for the GPL text.
Quiet it a bit by requiring --strict for the GPL bit.

Also make the address tests match a few uses of abbreviations
for street names and make it case insensitive.

Signed-off-by: Joe Perches <joe@perches.com>
Reviewed-by: Josh Triplett <josh@joshtriplett.org>
---
 scripts/checkpatch.pl | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c12234e..abd1811 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1970,15 +1970,16 @@ sub process {
 		}
 
 # Check for FSF mailing addresses.
-		if ($rawline =~ /You should have received a copy/ ||
-		    $rawline =~ /write to the Free Software/ ||
-		    $rawline =~ /59 Temple Place/ ||
-		    $rawline =~ /51 Franklin Street/) {
+		if ($rawline =~ /\bYou should have received a copy/i ||
+		    $rawline =~ /\bwrite to the Free/i ||
+		    $rawline =~ /\b59\s+Temple\s+Pl/i ||
+		    $rawline =~ /\b51\s+Franklin\s+St/i) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
 			my $msg_type = \&ERROR;
 			$msg_type = \&CHK if ($file);
+			$msg_type = \&CHK if ($rawline =~ /\bYou should have received a copy/i);
 			&{$msg_type}("FSF_MAILING_ADDRESS",
-				"Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The FSF has changed addresses in the past, and may do so again. Linux already includes a copy of the GPL.\n" . $herevet)
+				     "Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The FSF has changed addresses in the past, and may do so again. Linux already includes a copy of the GPL.\n" . $herevet)
 		}
 
 # check for Kconfig help text having a real description
-- 
1.8.1.2.459.gbcd45b4.dirty