[PATCH 0/7] update checkpatch to v0.30

[PATCH 0/7] update checkpatch to v0.30

[Andy Whitcroft]

Here is a batch of checkpatch updates.  The second of which should fix
akpm's issue triggering perl warnings on some patches.  The rest are the
normal slew of little fixes.  These have passed testing here.

Complete changelog below.

-apw

Andy Whitcroft (7):
  checkpatch: possible types -- prevent illegal modifiers being added
  checkpatch: correctly stop scanning at the bottom of a hunk
  checkpatch: update copyright dates
  checkpatch: fix false errors due to macro concatenation
  checkpatch: fix __attribute__ matching
  checkpatch: fix false EXPORT_SYMBOL warning
  checkpatch: version 0.30

 scripts/checkpatch.pl |   84 +++++++++++++++++++++++++++++++++++-------------
 1 files changed, 61 insertions(+), 23 deletions(-)

[PATCH 1/7] checkpatch: possible types -- prevent illegal modifiers being added

[Andy Whitcroft]

Prevent known non types being detected as modifiers.  Ensure we do not
look at any type which starts with a keyword.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   20 ++++++++++++--------
 1 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 87bbb8b..b43e309 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -997,23 +997,25 @@ sub annotate_values {
 
 sub possible {
 	my ($possible, $line) = @_;
-
-	print "CHECK<$possible> ($line)\n" if ($dbg_possible > 2);
-	if ($possible !~ /(?:
+	my $notPermitted = qr{(?:
 		^(?:
 			$Modifier|
 			$Storage|
 			$Type|
-			DEFINE_\S+|
+			DEFINE_\S+
+		)$|
+		^(?:
 			goto|
 			return|
 			case|
 			else|
 			asm|__asm__|
 			do
-		)$|
+		)(?:\s|$)|
 		^(?:typedef|struct|enum)\b
-	    )/x) {
+	    )}x;
+	warn "CHECK<$possible> ($line)\n" if ($dbg_possible > 2);
+	if ($possible !~ $notPermitted) {
 		# Check for modifiers.
 		$possible =~ s/\s*$Storage\s*//g;
 		$possible =~ s/\s*$Sparse\s*//g;
@@ -1022,8 +1024,10 @@ sub possible {
 		} elsif ($possible =~ /\s/) {
 			$possible =~ s/\s*$Type\s*//g;
 			for my $modifier (split(' ', $possible)) {
-				warn "MODIFIER: $modifier ($possible) ($line)\n" if ($dbg_possible);
-				push(@modifierList, $modifier);
+				if ($modifier !~ $notPermitted) {
+					warn "MODIFIER: $modifier ($possible) ($line)\n" if ($dbg_possible);
+					push(@modifierList, $modifier);
+				}
 			}
 
 		} else {
-- 
1.6.3.3

[PATCH 2/7] checkpatch: correctly stop scanning at the bottom of a hunk

[Andy Whitcroft]

We are allowing context scanning checks to apply against the first line
of context outside at the end of the hunk.  This can lead to false
matches to patch names leading to various perl warnings.  Correctly stop
at the bottom of the hunk.

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b43e309..1eca1e1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1234,7 +1234,6 @@ sub process {
 		$linenr++;
 
 		my $rawline = $rawlines[$linenr - 1];
-		my $hunk_line = ($realcnt != 0);
 
 #extract the line range in the file after the patch is applied
 		if ($line=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
@@ -1274,6 +1273,8 @@ sub process {
 			$realcnt--;
 		}
 
+		my $hunk_line = ($realcnt != 0);
+
 #make up the handle for any error we report on this line
 		$prefix = "$filename:$realline: " if ($emacs && $file);
 		$prefix = "$filename:$linenr: " if ($emacs && !$file);
-- 
1.6.3.3

[PATCH 3/7] checkpatch: update copyright dates

[Andy Whitcroft]

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1eca1e1..05b10d6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2,7 +2,7 @@
 # (c) 2001, Dave Jones. <davej@redhat.com> (the file handling bit)
 # (c) 2005, Joel Schopp <jschopp@austin.ibm.com> (the ugly bit)
 # (c) 2007,2008, Andy Whitcroft <apw@uk.ibm.com> (new conditions, test suite)
-# (c) 2008, Andy Whitcroft <apw@canonical.com>
+# (c) 2008,2009, Andy Whitcroft <apw@canonical.com>
 # Licensed under the terms of the GNU GPL License version 2
 
 use strict;
-- 
1.6.3.3

[PATCH 4/7] checkpatch: fix false errors due to macro concatenation

[Andy Whitcroft]

The macro concatenation (##) sequence can cause false errors when checking
macro's. Checkpatch doesn't currently know about the operator.

For example this line,

+ 	entry = (struct ftrace_raw_##call *)raw_data;                   \

is correct but it produces the following error,

ERROR: need consistent spacing around '*' (ctx:WxB)
+       entry = (struct ftrace_raw_##call *)raw_data;\
                                          ^

The line above doesn't have any spacing problems, and if you remove the
macro concatenation sequence checkpatch doesn't give any errors.

Extend identifier handling to include ## concatenation within the
definition of an identifier.

Reported-by: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 05b10d6..cb2dac3 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -130,7 +130,10 @@ if ($tree) {
 
 my $emitted_corrupt = 0;
 
-our $Ident       = qr{[A-Za-z_][A-Za-z\d_]*};
+our $Ident	= qr{
+			[A-Za-z_][A-Za-z\d_]*
+			(?:\s*\#\#\s*[A-Za-z_][A-Za-z\d_]*)*
+		}x;
 our $Storage	= qr{extern|static|asmlinkage};
 our $Sparse	= qr{
 			__user|
-- 
1.6.3.3

[PATCH 5/7] checkpatch: fix __attribute__ matching

[Andy Whitcroft]

In the following code,

union thread_union init_thread_union
	__attribute__((__section__(".data.init_task"))) =
		{ INIT_THREAD_INFO(init_task) };

There is a non-conforming declaration. It should really be like the
following,

union thread_union init_thread_union
	__attribute__((__section__(".data.init_task"))) = {
		INIT_THREAD_INFO(init_task)
};

However, checkpatch doesn't catch this right now because it doesn't
correctly evaluate the "__attribute__".

It is not at all clear that we care what preceeds an assignment style
attribute when we find the open brace.  Relax the test so we do not need
to check the __attribute__.

Reported-by: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index cb2dac3..ba105a8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1669,8 +1669,8 @@ sub process {
 		}
 
 # check for initialisation to aggregates open brace on the next line
-		if ($prevline =~ /$Declare\s*$Ident\s*=\s*$/ &&
-		    $line =~ /^.\s*{/) {
+		if ($line =~ /^.\s*{/ &&
+		    $prevline =~ /(?:^|[^=])=\s*$/) {
 			ERROR("that open brace { should be on the previous line\n" . $hereprev);
 		}
 
-- 
1.6.3.3

[PATCH 6/7] checkpatch: fix false EXPORT_SYMBOL warning

[Andy Whitcroft]

Ingo reported that the following lines triggered a false warning,

static struct lock_class_key rcu_lock_key;
struct lockdep_map rcu_lock_map =
        STATIC_LOCKDEP_MAP_INIT("rcu_read_lock", &rcu_lock_key);
EXPORT_SYMBOL_GPL(rcu_lock_map);

from kernel/rcutree.c , and the false warning looked like this,

WARNING: EXPORT_SYMBOL(foo); should immediately follow its
function/variable
+EXPORT_SYMBOL_GPL(rcu_lock_map);

We actually should be checking the statement before the EXPORT_* for a
mention of the exported object, and complain where it is not there.

Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Reported-by: Daniel Walker <dwalker@fifo99.com>
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |   48 +++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 39 insertions(+), 9 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ba105a8..07678c1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1145,6 +1145,7 @@ sub process {
 	# suppression flags
 	my %suppress_ifbraces;
 	my %suppress_whiletrailers;
+	my %suppress_export;
 
 	# Pre-scan the patch sanitizing the lines.
 	# Pre-scan the patch looking for any __setup documentation.
@@ -1253,6 +1254,7 @@ sub process {
 
 			%suppress_ifbraces = ();
 			%suppress_whiletrailers = ();
+			%suppress_export = ();
 			next;
 
 # track the line number as we move through the hunk, note that
@@ -1428,13 +1430,22 @@ sub process {
 		}
 
 # Check for potential 'bare' types
-		my ($stat, $cond, $line_nr_next, $remain_next, $off_next);
+		my ($stat, $cond, $line_nr_next, $remain_next, $off_next,
+		    $realline_next);
 		if ($realcnt && $line =~ /.\s*\S/) {
 			($stat, $cond, $line_nr_next, $remain_next, $off_next) =
 				ctx_statement_block($linenr, $realcnt, 0);
 			$stat =~ s/\n./\n /g;
 			$cond =~ s/\n./\n /g;
 
+			# Find the real next line.
+			$realline_next = $line_nr_next;
+			if (defined $realline_next &&
+			    (!defined $lines[$realline_next - 1] ||
+			     substr($lines[$realline_next - 1], $off_next) =~ /^\s*$/)) {
+				$realline_next++;
+			}
+
 			my $s = $stat;
 			$s =~ s/{.*$//s;
 
@@ -1695,21 +1706,40 @@ sub process {
 		$line =~ s@//.*@@;
 		$opline =~ s@//.*@@;
 
-#EXPORT_SYMBOL should immediately follow its function closing }.
-		if (($line =~ /EXPORT_SYMBOL.*\((.*)\)/) ||
-		    ($line =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {
+# EXPORT_SYMBOL should immediately follow the thing it is exporting, consider
+# the whole statement.
+#print "APW <$lines[$realline_next - 1]>\n";
+		if (defined $realline_next &&
+		    exists $lines[$realline_next - 1] &&
+		    !defined $suppress_export{$realline_next} &&
+		    ($lines[$realline_next - 1] =~ /EXPORT_SYMBOL.*\((.*)\)/ ||
+		     $lines[$realline_next - 1] =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {
 			my $name = $1;
-			if ($prevline !~ /(?:
-				^.}|
+			if ($stat !~ /(?:
+				\n.}\s*$|
 				^.DEFINE_$Ident\(\Q$name\E\)|
 				^.DECLARE_$Ident\(\Q$name\E\)|
 				^.LIST_HEAD\(\Q$name\E\)|
-				^.$Type\s*\(\s*\*\s*\Q$name\E\s*\)\s*\(|
-				\b\Q$name\E(?:\s+$Attribute)?\s*(?:;|=|\[)
+				^.(?:$Storage\s+)?$Type\s*\(\s*\*\s*\Q$name\E\s*\)\s*\(|
+				\b\Q$name\E(?:\s+$Attribute)*\s*(?:;|=|\[|\()
 			    )/x) {
-				WARN("EXPORT_SYMBOL(foo); should immediately follow its function/variable\n" . $herecurr);
+#print "FOO A<$lines[$realline_next - 1]> stat<$stat> name<$name>\n";
+				$suppress_export{$realline_next} = 2;
+			} else {
+				$suppress_export{$realline_next} = 1;
 			}
 		}
+		if (!defined $suppress_export{$linenr} &&
+		    $prevline =~ /^.\s*$/ &&
+                    ($line =~ /EXPORT_SYMBOL.*\((.*)\)/ ||
+		     $line =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {
+#print "FOO B <$lines[$linenr - 1]>\n";
+			$suppress_export{$linenr} = 2;
+		}
+		if (defined $suppress_export{$linenr} &&
+		    $suppress_export{$linenr} == 2) {
+			WARN("EXPORT_SYMBOL(foo); should immediately follow its function/variable\n" . $herecurr);
+		}
 
 # check for external initialisers.
 		if ($line =~ /^.$Type\s*$Ident\s*(?:\s+$Modifier)*\s*=\s*(0|NULL|false)\s*;/) {
-- 
1.6.3.3

Re: [PATCH 6/7] checkpatch: fix false EXPORT_SYMBOL warning

[Daniel Walker]

On Fri, 2009-10-16 at 19:39 +0100, Andy Whitcroft wrote:

> -				WARN("EXPORT_SYMBOL(foo); should immediately follow its function/variable\n" . $herecurr);
> +#print "FOO A<$lines[$realline_next - 1]> stat<$stat> name<$name>\n";
> +				$suppress_export{$realline_next} = 2;
> +			} else {
> +				$suppress_export{$realline_next} = 1;
>  			}
>  		}
> +		if (!defined $suppress_export{$linenr} &&
> +		    $prevline =~ /^.\s*$/ &&
> +                    ($line =~ /EXPORT_SYMBOL.*\((.*)\)/ ||
> +		     $line =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {

ERROR: code indent should use tabs where possible
#148: FILE: scripts/checkpatch.pl:1734:
+                    ($line =~ /EXPORT_SYMBOL.*\((.*)\)/ ||$

This one line is spaced over instead of tabbed.. I guess checkpatch can
even check itself, which is nice .. I'm not sure this matters since this
it's perl tho..

Do you apply any kind of coding style to checkpatch itself ?

Daniel

Re: [PATCH 6/7] checkpatch: fix false EXPORT_SYMBOL warning

[Andy Whitcroft]

On Fri, Oct 16, 2009 at 11:58:21AM -0700, Daniel Walker wrote:
> On Fri, 2009-10-16 at 19:39 +0100, Andy Whitcroft wrote:
> 
> > -				WARN("EXPORT_SYMBOL(foo); should immediately follow its function/variable\n" . $herecurr);
> > +#print "FOO A<$lines[$realline_next - 1]> stat<$stat> name<$name>\n";
> > +				$suppress_export{$realline_next} = 2;
> > +			} else {
> > +				$suppress_export{$realline_next} = 1;
> >  			}
> >  		}
> > +		if (!defined $suppress_export{$linenr} &&
> > +		    $prevline =~ /^.\s*$/ &&
> > +                    ($line =~ /EXPORT_SYMBOL.*\((.*)\)/ ||
> > +		     $line =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {
> 
> ERROR: code indent should use tabs where possible
> #148: FILE: scripts/checkpatch.pl:1734:
> +                    ($line =~ /EXPORT_SYMBOL.*\((.*)\)/ ||$
> 
> This one line is spaced over instead of tabbed.. I guess checkpatch can
> even check itself, which is nice .. I'm not sure this matters since this
> it's perl tho..
> 
> Do you apply any kind of coding style to checkpatch itself ?

Yeah normally I checkpatch it, and ignore the width issues as the long
regexps are not always sensibly wrappable.  I rushed this lot out as I
am travelling tommorrow.  Sigh.  Never rush out a set of patches.

-apw

[PATCH 7/7] checkpatch: version 0.30

[Andy Whitcroft]

Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 07678c1..9f5c1b7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -10,7 +10,7 @@ use strict;
 my $P = $0;
 $P =~ s@.*/@@g;
 
-my $V = '0.29';
+my $V = '0.30';
 
 use Getopt::Long qw(:config no_auto_abbrev);
 
-- 
1.6.3.3

Re: [PATCH 0/7] update checkpatch to v0.30

[Joe Perches]

On Fri, 2009-10-16 at 19:39 +0100, Andy Whitcroft wrote:
> Here is a batch of checkpatch updates.  The second of which should fix
> akpm's issue triggering perl warnings on some patches.  The rest are the
> normal slew of little fixes.  These have passed testing here.

Andy, did you get my patches to checkpatch
or did you decide not to apply them?

http://patchwork.kernel.org/patch/51566/
http://patchwork.kernel.org/patch/51486/

Re: [PATCH 0/7] update checkpatch to v0.30

[Andy Whitcroft]

On Fri, Oct 16, 2009 at 11:56:39AM -0700, Joe Perches wrote:
> On Fri, 2009-10-16 at 19:39 +0100, Andy Whitcroft wrote:
> > Here is a batch of checkpatch updates.  The second of which should fix
> > akpm's issue triggering perl warnings on some patches.  The rest are the
> > normal slew of little fixes.  These have passed testing here.
> 
> Andy, did you get my patches to checkpatch
> or did you decide not to apply them?
> 
> http://patchwork.kernel.org/patch/51566/
> http://patchwork.kernel.org/patch/51486/

Missed them, I'll hoover them up tommorrow.

Thanks for the prod.

-apw