checkpatch: update the 80-char-line check to allow for long strings
PATCH 1/1 checkpatch: update the 80-char-line check to allow for long strings
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 <[email protected]>
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
--- scripts/checkpatch.pl | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
scripts/checkpatch.pl +1 -1
@@ -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); }
Signature · 1 line
1.5.5.1
Quoted 6 lines · 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?
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.. :-)
Quoted 5 lines · On Wed, 14 May 2008 09:49:06 +0200 Peter Zijlstra <[email protected]> wrote:
Quoted 4 lines · 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.
Quoted 7 lines
Quoted 2 lines
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.
Quoted 11 lines · Andres Salomon
Quoted 8 lines · On Wed, 14 May 2008 09:49:06 +0200 Peter Zijlstra <[email protected]> wrote:
Quoted 6 lines · 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?
Quoted 8 lines
Quoted 4 lines
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
Quoted 4 lines · On Wed, 14 May 2008 16:59:26 +0100 Andy Whitcroft <[email protected]> wrote:
Quoted 3 lines · Andres Salomon
On Wed, 14 May 2008 09:49:06 +0200
Peter Zijlstra <[email protected]> wrote:
[...]
Quoted 5 lines
Quoted 4 lines
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.
Quoted 7 lines
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...