[PATCH] checkpatch: update the 80-char-line check to allow for long strings

[PATCH] checkpatch: update the 80-char-line check to allow for long strings

[Andres Salomon]

19:29 * dilinger sends his patch to lkml for a proper flaming
19:32  Quozl> dilinger: it's called "patch hardening"?  ;-}  the heat
              of the flames makes the patch stronger.

I've wasted too much time massaging printk strings in order to satisfy
checkpatch.pl.  Comments on this?





From: Andres Salomon <dilinger@debian.org>


Checkpatch.pl is pretty strict in its lines-must-be-less-than-80-chars
check.  I consider this a good thing in most scenarios; however,
when it comes to strings, I don't consider the following to be readable:

       printk(KERN_WARNING
                       "one two three "
                       "fooooooooooooooooooooooooooooooooooooooooooooooo\n");

CodingStyle backs me up on this (if I'm interpreting that section clearly).
Ingo[0] has also pointed out why this might be a bad thing as well.

This patch is an attempt to make checkpatch.pl more useful.  Folks ignore
long string warnings[1], which is bad form; we don't _want_ people getting
used to ignoring checkpatch.pl warnings any more than we want people
getting used to ignoring compiler warnings.

The patch updates checkpatch.pl to ignore lines that are > 80 chars
if they contain only (quoted) strings and perhaps some additional stuff
at the end.  For example, the following no longer has warnings emitted:

+ printk(KERN_WARNING
+                       "one two three fooooooooooooooooooooooooooooooooooooooooooooooo\n");

+       printk(KERN_WARNING
+                       "one two three fooooooooooooooooooooooooooooooooooooooooo\n",
+                       xyz);

However, the following still triggers checkpatch.pl warnings:

+ printk(KERN_WARNING "one two three fooooooooooooooooooooooooooooooooooooooooooooooo\n");

+       printk(KERN_WARNING
+                       "one two three foooooooooooooooooooooooooooooooooooooo\n", xyz);

I'm sure it's possible to fool the check, but we're not trying to guard
against malicious patch authors.

[0] http://thread.gmane.org/gmane.linux.kernel/679732/focus=679760
[1] http://lists.laptop.org/pipermail/devel/2008-May/014121.html

Signed-off-by: Andres Salomon <dilinger@debian.org>
---
 scripts/checkpatch.pl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index b6bbbcd..00f3d05 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1117,7 +1117,7 @@ sub process {
 			ERROR("trailing whitespace\n" . $herevet);
 		}
 #80 column limit
-		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && $length > 80) {
+		if ($line =~ /^\+/ && !($prevrawline=~/\/\*\*/) && !($line =~ /^\+\s*"/ && $line =~ /"[);,\s]*$/) && $length > 80) {
 			WARN("line over 80 characters\n" . $herecurr);
 		}
 
-- 
1.5.5.1

Re: [PATCH] checkpatch: update the 80-char-line check to allow for long strings

[Peter Zijlstra]

On Wed, 2008-05-14 at 00:53 -0400, Andres Salomon wrote:
> 19:29 * dilinger sends his patch to lkml for a proper flaming
> 19:32  Quozl> dilinger: it's called "patch hardening"?  ;-}  the heat
>               of the flames makes the patch stronger.
> 
> I've wasted too much time massaging printk strings in order to satisfy
> checkpatch.pl.  Comments on this?

How about treating like its meant to be; as a guide instead of a hard
rule. That means you can ignore it with good taste.. :-)

Re: [PATCH] checkpatch: update the 80-char-line check to allow for long strings

[Andres Salomon]

On Wed, 14 May 2008 09:49:06 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, 2008-05-14 at 00:53 -0400, Andres Salomon wrote:
> > 19:29 * dilinger sends his patch to lkml for a proper flaming
> > 19:32  Quozl> dilinger: it's called "patch hardening"?  ;-}  the
> > heat of the flames makes the patch stronger.
> > 
> > I've wasted too much time massaging printk strings in order to
> > satisfy checkpatch.pl.  Comments on this?
> 
> How about treating like its meant to be; as a guide instead of a hard
> rule. That means you can ignore it with good taste.. :-)
> 
> 

The point is that we tell people to make sure that their patches are
checkpatch-clean.  Let's make the tool reflect what we're actually
looking for.

Re: [PATCH] checkpatch: update the 80-char-line check to allow for long strings

[Andy Whitcroft]

On Wed, May 14, 2008 at 10:49:38AM -0400, Andres Salomon wrote:
> On Wed, 14 May 2008 09:49:06 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Wed, 2008-05-14 at 00:53 -0400, Andres Salomon wrote:
> > > 19:29 * dilinger sends his patch to lkml for a proper flaming
> > > 19:32  Quozl> dilinger: it's called "patch hardening"?  ;-}  the
> > > heat of the flames makes the patch stronger.
> > > 
> > > I've wasted too much time massaging printk strings in order to
> > > satisfy checkpatch.pl.  Comments on this?
> > 
> > How about treating like its meant to be; as a guide instead of a hard
> > rule. That means you can ignore it with good taste.. :-)
> > 
> > 
> 
> The point is that we tell people to make sure that their patches are
> checkpatch-clean.  Let's make the tool reflect what we're actually
> looking for.

We did have a mini discussion on this when Ingo suggested allowing the
strings to run off the end of the line so that grep would work.  I did
put together a patch then for this.  There was no real response to the
suggestion we should take that recommendation into CodingStyle and thus
into checkpatch.

-apw

Re: [PATCH] checkpatch: update the 80-char-line check to allow for long strings

[Andres Salomon]

On Wed, 14 May 2008 16:59:26 +0100
Andy Whitcroft <apw@shadowen.org> wrote:

> On Wed, May 14, 2008 at 10:49:38AM -0400, Andres Salomon wrote:
> > On Wed, 14 May 2008 09:49:06 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
[...]
> > 
> > The point is that we tell people to make sure that their patches are
> > checkpatch-clean.  Let's make the tool reflect what we're actually
> > looking for.
> 
> We did have a mini discussion on this when Ingo suggested allowing the
> strings to run off the end of the line so that grep would work.  I did
> put together a patch then for this.  There was no real response to the
> suggestion we should take that recommendation into CodingStyle and
> thus into checkpatch.
> 
> -apw

I've yet to hear anyone say that this idea is awful...