This update brings a couple of new checks, and a number of fixes. Of note:
- allows parentheses on return of comparisons
- tracks #ifdef sections better
- fixes miss reports of variables containing static
It also updates the maintainers entry to track my new email address and
to drop inactive maintainers.
Complete changelog below.
-apw
Andy Whitcroft (11):
checkpatch: update MAINTAINERS entry
checkpatch: update copyrights
checkpatch: allow parentheses on return for comparisons
checkpatch: loosen spacing on typedef function checks
checkpatch: fix continuation detection when handling spacing on operators
checkpatch: track #ifdef/#else/#endif when tracking blocks
checkpatch: do not report nr_static as a static declaration
checkpatch: ensure we actually detect if assignments split across lines
checkpatch: struct file_operations should normally be const
checkpatch: fix the perlcritic errors
checkpatch: version: 0.26
Mike Frysinger (1):
checkpatch: try to catch missing VMLINUX_SYMBOL() in vmlinux.lds.h
Wolfram Sang (1):
checkpatch: Add warning for p0-patches
scripts/checkpatch.pl | 84 +++++++++++++++++++++++++++++++++++++++----------
1 files changed, 67 insertions(+), 17 deletions(-)
Update my email address to my new work address. Also, as per our recent
email conversation remove Randy and Joel from the maintainers list.
Finally add LKML so that emails are recorded somewhere.
Signed-off-by: Andy Whitcroft <[email protected]>
Cc: Randy Dunlap <[email protected]>
Cc: Joel Schopp <[email protected]>
---
MAINTAINERS | 7 ++-----
1 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index 618c1ef..0d587ff 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1091,11 +1091,8 @@ S: Maintained
CHECKPATCH
P: Andy Whitcroft
-M: [email protected]
-P: Randy Dunlap
-M: [email protected]
-P: Joel Schopp
-M: [email protected]
+M: [email protected]
+L: [email protected]
S: Supported
CISCO 10G ETHERNET DRIVER
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 770fa41..f015698 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1,7 +1,8 @@
#!/usr/bin/perl -w
# (c) 2001, Dave Jones. <[email protected]> (the file handling bit)
# (c) 2005, Joel Schopp <[email protected]> (the ugly bit)
-# (c) 2007, Andy Whitcroft <[email protected]> (new conditions, test suite, etc)
+# (c) 2007,2008, Andy Whitcroft <[email protected]> (new conditions, test suite)
+# (c) 2008, Andy Whitcroft <[email protected]>
# Licensed under the terms of the GNU GPL License version 2
use strict;
--
1.6.0.4.911.gc990
From: Wolfram Sang <[email protected]>
Some people work internally with -p0-patches which has the danger that
one forgets to convert them to -p1 before mainlining. Bitten myself and
seen p0-patches in mailing lists occasionally, this patch adds a warning
to checkpatch.pl in case a patch is -p0. If you really want, you can
fool this check to generate false positives, this is why it just spits a
warning. Making the check 100% proof is trickier than it looks, so let's
start with a version which catches the cases of real use.
[[email protected]: update message language, handle null prefix, add tests]
Signed-off-by: Wolfram Sang <[email protected]>
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f015698..b953c76 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1057,6 +1057,7 @@ sub process {
my $in_comment = 0;
my $comment_edge = 0;
my $first_line = 0;
+ my $p1_prefix = '';
my $prev_values = 'E';
@@ -1205,7 +1206,12 @@ sub process {
# extract the filename as it passes
if ($line=~/^\+\+\+\s+(\S+)/) {
$realfile = $1;
- $realfile =~ s@^[^/]*/@@;
+ $realfile =~ s@^([^/]*)/@@;
+
+ $p1_prefix = $1;
+ if ($tree && $p1_prefix ne '' && -e "$root/$p1_prefix") {
+ WARN("patch prefix '$p1_prefix' exists, appears to be a -p0 patch\n");
+ }
if ($realfile =~ m@^include/asm/@) {
ERROR("do not modify files in include/asm, change architecture specific files in include/asm-<architecture>\n" . "$here$rawline\n");
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
It seems to be a common idiom to include braces on conditionals in all
contexts including return. Allow this exception to the return is not
a function checks. Reported by Kay Sievers.
Cc: Kay Sievers <[email protected]>
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b953c76..5c7fd1a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -127,6 +127,7 @@ our $Lval = qr{$Ident(?:$Member)*};
our $Constant = qr{(?:[0-9]+|0x[0-9a-fA-F]+)[UL]*};
our $Assignment = qr{(?:\*\=|/=|%=|\+=|-=|<<=|>>=|&=|\^=|\|=|=)};
+our $Compare = qr{<=|>=|==|!=|<|>};
our $Operators = qr{
<=|>=|==|!=|
=>|->|<<|>>|<|>|!|~|
@@ -1983,9 +1984,9 @@ sub process {
my $spacing = $1;
my $value = $2;
- # Flatten any parentheses and braces
+ # Flatten any parentheses
$value =~ s/\)\(/\) \(/g;
- while ($value =~ s/\([^\(\)]*\)/1/) {
+ while ($value !~ /(?:$Ident|-?$Constant)\s*$Compare\s*(?:$Ident|-?$Constant)/ && $value =~ s/\([^\(\)]*\)/1/) {
}
if ($value =~ /^(?:$Ident|-?$Constant)$/) {
--
1.6.0.4.911.gc990
From: Mike Frysinger <[email protected]>
Seems like every other release we have someone who updates vmlinux.lds.h and
adds C-visible symbols without VMLINUX_SYMBOL() around them. So start
checking the file and reject assignments which have plain symbols on either
side.
[[email protected]: soften the check, add tests]
Signed-off-by: Mike Frysinger <[email protected]>
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 5c7fd1a..705a043 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2224,6 +2224,15 @@ sub process {
}
}
+# make sure symbols are always wrapped with VMLINUX_SYMBOL() ...
+# all assignments may have only one of the following with an assignment:
+# .
+# ALIGN(...)
+# VMLINUX_SYMBOL(...)
+ if ($realfile eq 'vmlinux.lds.h' && $line =~ /(?:(?:^|\s)$Ident\s*=|=\s*$Ident(?:\s|$))/) {
+ WARN("vmlinux.lds.h needs VMLINUX_SYMBOL() around C-visible symbols\n" . $herecurr);
+ }
+
# check for redundant bracing round if etc
if ($line =~ /(^.*)\bif\b/ && $1 !~ /else\s*$/) {
my ($level, $endln, @chunks) =
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
Loosen spacing checks to correctly detect this valid use of a typedef:
typedef struct rcu_data *(*get_data_func)(int);
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 705a043..d80b55a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1618,7 +1618,7 @@ sub process {
# check for new typedefs, only function parameters and sparse annotations
# make sense.
if ($line =~ /\btypedef\s/ &&
- $line !~ /\btypedef\s+$Type\s+\(\s*\*?$Ident\s*\)\s*\(/ &&
+ $line !~ /\btypedef\s+$Type\s*\(\s*\*?$Ident\s*\)\s*\(/ &&
$line !~ /\btypedef\s+$Type\s+$Ident\s*\(/ &&
$line !~ /\b$typeTypedefs\b/ &&
$line !~ /\b__bitwise(?:__|)\b/) {
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
We are miss categorising a continuation fragment following an operator
which may lead to us thinking that there is a space after it when there
is not. Fix this up.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d80b55a..67b0c9f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1793,7 +1793,7 @@ sub process {
$c = 'C' if ($elements[$n + 2] =~ /^$;/);
$c = 'B' if ($elements[$n + 2] =~ /^(\)|\]|;)/);
$c = 'O' if ($elements[$n + 2] eq '');
- $c = 'E' if ($elements[$n + 2] =~ /\s*\\$/);
+ $c = 'E' if ($elements[$n + 2] =~ /^\s*\\$/);
} else {
$c = 'E';
}
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
When picking up a complete statement or block for analysis we cannot
simply track open/close/etc parenthesis we must take into account
preprocessor section boundaries.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 22 ++++++++++++++++++++++
1 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 67b0c9f..906624c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -405,6 +405,7 @@ sub ctx_statement_block {
my $type = '';
my $level = 0;
+ my @stack = ([$type, $level]);
my $p;
my $c;
my $len = 0;
@@ -436,6 +437,16 @@ sub ctx_statement_block {
$remainder = substr($blk, $off);
#warn "CSB: c<$c> type<$type> level<$level> remainder<$remainder> coff_set<$coff_set>\n";
+
+ # Handle nested #if/#else.
+ if ($remainder =~ /^#\s*(?:ifndef|ifdef|if)\s/) {
+ push(@stack, [ $type, $level ]);
+ } elsif ($remainder =~ /^#\s*(?:else|elif)\b/) {
+ ($type, $level) = @{$stack[$#stack - 1]};
+ } elsif ($remainder =~ /^#\s*endif\b/) {
+ ($type, $level) = @{pop(@stack)};
+ }
+
# Statement ends at the ';' or a close '}' at the
# outermost level.
if ($level == 0 && $c eq ';') {
@@ -582,11 +593,22 @@ sub ctx_block_get {
my @res = ();
my $level = 0;
+ my @stack = ($level);
for ($line = $start; $remain > 0; $line++) {
next if ($rawlines[$line] =~ /^-/);
$remain--;
$blk .= $rawlines[$line];
+
+ # Handle nested #if/#else.
+ if ($rawlines[$line] =~ /^.\s*#\s*(?:ifndef|ifdef|if)\s/) {
+ push(@stack, $level);
+ } elsif ($rawlines[$line] =~ /^.\s*#\s*(?:else|elif)\b/) {
+ $level = $stack[$#stack - 1];
+ } elsif ($rawlines[$line] =~ /^.\s*#\s*endif\b/) {
+ $level = pop(@stack);
+ }
+
foreach my $c (split(//, $rawlines[$line])) {
##print "C<$c>L<$level><$open$close>O<$off>\n";
if ($off > 0) {
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
Ensure we do not report identifiers containing the word static as static
declarations. For example this should not be reported as an unecessary
assignement of 0:
long nr_static = 0;
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 906624c..a521d49 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1632,7 +1632,7 @@ sub process {
$herecurr);
}
# check for static initialisers.
- if ($line =~ /\s*static\s.*=\s*(0|NULL|false)\s*;/) {
+ if ($line =~ /\bstatic\s.*=\s*(0|NULL|false)\s*;/) {
ERROR("do not initialise statics to 0 or NULL\n" .
$herecurr);
}
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
When checking for assignments within if conditionals we check the whole
of the condition, but the match is performed using a line constrained
regular expression. This means we can miss split conditionals or those
on the second line. Allow the check to span lines.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a521d49..c39ce0b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2048,7 +2048,7 @@ sub process {
$line =~ /\b(?:if|while|for)\s*\(/ && $line !~ /^.\s*#/) {
my ($s, $c) = ($stat, $cond);
- if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/) {
+ if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/s) {
ERROR("do not use assignment in if condition\n" . $herecurr);
}
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
Clean up checkpatch using perlcritic.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 20 +++++++++++++-------
1 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 94371f6..bd6ac90 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -69,7 +69,9 @@ my $dbg_possible = 0;
my $dbg_type = 0;
my $dbg_attr = 0;
for my $key (keys %debug) {
- eval "\${dbg_$key} = '$debug{$key}';"
+ ## no critic
+ eval "\${dbg_$key} = '$debug{$key}';";
+ die "$@" if ($@);
}
if ($terse) {
@@ -206,9 +208,9 @@ my @dep_includes = ();
my @dep_functions = ();
my $removal = "Documentation/feature-removal-schedule.txt";
if ($tree && -f "$root/$removal") {
- open(REMOVE, "<$root/$removal") ||
+ open(my $REMOVE, '<', "$root/$removal") ||
die "$P: $removal: open failed - $!\n";
- while (<REMOVE>) {
+ while (<$REMOVE>) {
if (/^Check:\s+(.*\S)/) {
for my $entry (split(/[, ]+/, $1)) {
if ($entry =~ m@include/(.*)@) {
@@ -220,17 +222,21 @@ if ($tree && -f "$root/$removal") {
}
}
}
+ close($REMOVE);
}
my @rawlines = ();
my @lines = ();
my $vname;
for my $filename (@ARGV) {
+ my $FILE;
if ($file) {
- open(FILE, "diff -u /dev/null $filename|") ||
+ open($FILE, '-|', "diff -u /dev/null $filename") ||
die "$P: $filename: diff failed - $!\n";
+ } elsif ($filename eq '-') {
+ open($FILE, '<&STDIN');
} else {
- open(FILE, "<$filename") ||
+ open($FILE, '<', "$filename") ||
die "$P: $filename: open failed - $!\n";
}
if ($filename eq '-') {
@@ -238,11 +244,11 @@ for my $filename (@ARGV) {
} else {
$vname = $filename;
}
- while (<FILE>) {
+ while (<$FILE>) {
chomp;
push(@rawlines, $_);
}
- close(FILE);
+ close($FILE);
if (!process($filename)) {
$exit = 1;
}
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bd6ac90..7bed4ed 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -10,7 +10,7 @@ use strict;
my $P = $0;
$P =~ s@.*/@@g;
-my $V = '0.25';
+my $V = '0.26';
use Getopt::Long qw(:config no_auto_abbrev);
--
1.6.0.4.911.gc990
From: Andy Whitcroft <[email protected]>
In the general use case struct file_operations should be a const object.
Check for and warn where it is not. As suggested by Steven and Ingo.
Cc: Steven Rostedt <[email protected]>
Cc: Ingo Molnar <[email protected]>
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c39ce0b..94371f6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2509,6 +2509,11 @@ sub process {
if ($line =~ /^.\s*__initcall\s*\(/) {
WARN("please use device_initcall() instead of __initcall()\n" . $herecurr);
}
+# check for struct file_operations, ensure they are const.
+ if ($line =~ /\bstruct\s+file_operations\b/ &&
+ $line !~ /\bconst\b/) {
+ WARN("struct file_operations should normally be const\n" . $herecurr);
+ }
# use of NR_CPUS is usually wrong
# ignore definitions of NR_CPUS and usage to define arrays as likely right
--
1.6.0.4.911.gc990
On Sun, Dec 07, 2008 at 06:30:48PM +0000, Andy Whitcroft wrote:
> From: Andy Whitcroft <[email protected]>
>
> In the general use case struct file_operations should be a const object.
> Check for and warn where it is not. As suggested by Steven and Ingo.
Andy,
Thanks for doing this!
Acked-by: Steven Rostedt <[email protected]>
-- Steve
>
> Cc: Steven Rostedt <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Signed-off-by: Andy Whitcroft <[email protected]>
> ---
> scripts/checkpatch.pl | 5 +++++
> 1 files changed, 5 insertions(+), 0 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index c39ce0b..94371f6 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2509,6 +2509,11 @@ sub process {
> if ($line =~ /^.\s*__initcall\s*\(/) {
> WARN("please use device_initcall() instead of __initcall()\n" . $herecurr);
> }
> +# check for struct file_operations, ensure they are const.
> + if ($line =~ /\bstruct\s+file_operations\b/ &&
> + $line !~ /\bconst\b/) {
> + WARN("struct file_operations should normally be const\n" . $herecurr);
> + }
>
> # use of NR_CPUS is usually wrong
> # ignore definitions of NR_CPUS and usage to define arrays as likely right
> --
> 1.6.0.4.911.gc990
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>