[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

Made the length test a variable for easier bike shedding.

Also add --strict tests for longer than 80 chars and more
than 6 leading tabs

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a4d7434..2c5f30c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -30,6 +30,10 @@ my $root;
 my %debug;
 my $help = 0;
 
+# Code style warnings
+my $style_long_line = 105;
+my $style_max_leading_tabs = 6;
+
 sub help {
 	my ($exitcode) = @_;
 
@@ -1385,13 +1389,21 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
-		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
+#Line too long
+		if ($line =~ /^\+/ &&
+		    $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ &&
-		    $length > 80)
-		{
-			WARN("line over 80 characters\n" . $herecurr);
+		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/) {
+			if ($length > $style_long_line) {
+				WARN("line over $style_long_line characters\n" . $herecurr);
+			} elsif ($length > 80) {
+				CHK("line over 80 characters\n" . $herecurr);
+			}
+		}
+
+#too many leading tabs - deep leading indent
+		if ($line =~ /^\+\t{$style_max_leading_tabs,}(?!(.*,$|.*\);$))/) {
+			CHK("Too many leading tabs.  Consider restructuring code\n" . $herecurr);
 		}
 
 # check for spaces before a quoted newline

[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

Hello Linus.

Could you pick up this patch directly please?

Andrew Morton doesn't want to as he believes using >80
column lines not nice, and Andy Whitcroft doesn't seem
to have much time to work on checkpatch these days.

Either this or perhaps just remove the >80 column test
altogether.

Thanks

-------------------------------------

Made the length test a variable for easier bike shedding.

Also add --strict tests for lines longer than 80 chars and
more than 6 leading tabs.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a4d7434..2c5f30c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -30,6 +30,10 @@ my $root;
 my %debug;
 my $help = 0;
 
+# Code style warnings
+my $style_long_line = 105;
+my $style_max_leading_tabs = 6;
+
 sub help {
 	my ($exitcode) = @_;
 
@@ -1385,13 +1389,21 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
-		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
+#Line too long
+		if ($line =~ /^\+/ &&
+		    $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ &&
-		    $length > 80)
-		{
-			WARN("line over 80 characters\n" . $herecurr);
+		    $line !~ /^\+\s*$logFunctions\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/) {
+			if ($length > $style_long_line) {
+				WARN("line over $style_long_line characters\n" . $herecurr);
+			} elsif ($length > 80) {
+				CHK("line over 80 characters\n" . $herecurr);
+			}
+		}
+
+#too many leading tabs - deep leading indent
+		if ($line =~ /^\+\t{$style_max_leading_tabs,}(?!(.*,$|.*\);$))/) {
+			CHK("Too many leading tabs.  Consider restructuring code\n" . $herecurr);
 		}
 
 # check for spaces before a quoted newline

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Andi Kleen]

Linus Torvalds <torvalds@linux-foundation.org> writes:

>
> I like this. Except I think the indent test should count spaces too some 
> way. Or do we already warn about excessive space that should be tabs?

checkpatch warns about those.

(it triggers for me all the time because those happen when you 
copy patch reject blocks from hand and remove the leading character
without removing the space afterwards)

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

On Fri, 2009-12-18 at 09:43 -0800, Linus Torvalds wrote:
> I like this. Except I think the indent test should count spaces too some 
> way. Or do we already warn about excessive space that should be tabs?

Andy's the checkpatch guy to verify this but:

There's a separate test for spaces that should be tabs.

If you had a 64 space indent and ran the silly script,
it'd complain about the spaces.

If you converted the spaces to tabs and ran the silly
script again with --strict, it'd complain about the
indent depth.

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Linus Torvalds]

On Fri, 18 Dec 2009, Paul Mundt wrote:

> On Thu, Dec 17, 2009 at 09:12:24PM -0800, Joe Perches wrote:
> > On Thu, 2009-12-17 at 23:29 -0500, Valdis.Kletnieks@vt.edu wrote:
> > > Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
> > > continuation line, it's probably in its rights to whinge. 
> > > 	grep -r -P '^\t{7:}(?!(.*,$|.*\);$))' . | more
> > > produces a whole lot of "this can't end well" output.
> > 
> > I think this is a good test to add to checkpatch.
> > 
> > Add 105 character long line WARN test
> > Add 80 character long line STRICT test
> > Add 6+ leading indent tabs test, consider restructuring
> > 
> This looks like a reasonable compromise.

I like this. Except I think the indent test should count spaces too some 
way. Or do we already warn about excessive space that should be tabs?

		Linus

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

On Fri, 2009-12-18 at 15:37 +0100, Krzysztof Halasa wrote:
> > Add 80 character long line STRICT test
> Not sure what do you mean.

CHK messages are only printed when --strict is on the command line

> This makes perfect sense, at least until shown otherwise (though being
> a warning instead of an error means it's ok to trigger in perfectly
> valid code).

It's a CHK, only printed with --strict.

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Krzysztof Halasa]

Joe Perches <joe@perches.com> writes:

> I think this is a good test to add to checkpatch.
>
> Add 105 character long line WARN test

Not sure if that's long enough (maybe it is).
132 looks like the next "natural" number.
The problematic case like printk(KERN_DEBUG "%s:%s:%s", var ? "yes" :
"no" and so on. If we split the code like that then better use 132 than
105 - even with 132 chars we may need to split, remember it's the printk
output which has to fit in 80 chars, or maybe usually has.

The main problem is checkpatch can't estimate "complexity". Fortunately
the reviewers can.

> Add 80 character long line STRICT test

Not sure what do you mean.

> Add 6+ leading indent tabs test, consider restructuring

This makes perfect sense, at least until shown otherwise (though being
a warning instead of an error means it's ok to trigger in perfectly
valid code).
-- 
Krzysztof Halasa

Re: [PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Paul Mundt]

On Thu, Dec 17, 2009 at 09:12:24PM -0800, Joe Perches wrote:
> On Thu, 2009-12-17 at 23:29 -0500, Valdis.Kletnieks@vt.edu wrote:
> > Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
> > continuation line, it's probably in its rights to whinge. 
> > 	grep -r -P '^\t{7:}(?!(.*,$|.*\);$))' . | more
> > produces a whole lot of "this can't end well" output.
> 
> I think this is a good test to add to checkpatch.
> 
> Add 105 character long line WARN test
> Add 80 character long line STRICT test
> Add 6+ leading indent tabs test, consider restructuring
> 
This looks like a reasonable compromise.

[PATCH] scripts/checkpatch.pl: Change long line warning to 105 chars

[Joe Perches]

On Thu, 2009-12-17 at 23:29 -0500, Valdis.Kletnieks@vt.edu wrote:
> Yeah, but I respectfully submit that if the regexp '^\t{6}' matches a non-
> continuation line, it's probably in its rights to whinge. 
> 	grep -r -P '^\t{7:}(?!(.*,$|.*\);$))' . | more
> produces a whole lot of "this can't end well" output.

I think this is a good test to add to checkpatch.

Add 105 character long line WARN test
Add 80 character long line STRICT test
Add 6+ leading indent tabs test, consider restructuring

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

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bc4114f..c358251 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1374,13 +1374,20 @@ sub process {
 # check we are in a valid source file if not then ignore this hunk
 		next if ($realfile !~ /\.(h|c|s|S|pl|sh)$/);
 
-#80 column limit
+#Line too long
 		if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
 		    $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
-		    $line !~ /^\+\s*printk\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ &&
-		    $length > 80)
-		{
-			WARN("line over 80 characters\n" . $herecurr);
+		    $line !~ /^\+\s*printk\s*\(\s*(?:KERN_\S+\s*)?"[X\t]*"\s*(?:,|\)\s*;)\s*$/) {
+			if ($length > 105) {
+				WARN("line over 105 characters\n" . $herecurr);
+			} elsif ($length > 80) {
+				CHK("line over 80 characters\n" . $herecurr);
+			}
+		}
+
+#too many leading tabs - deep leading indent
+		if ($line =~ /^\+	{6,}(?!(.*,$|.*\);$))/) {
+			CHK("Too many leading tabs.  Consider restructuring code\n" . $herecurr);
 		}
 
 # check for adding lines without a newline.