RFC: Update coding standard to avoid split up printk format strings
Common occurrence: You see some error message in the kernel log you don't
understand, Standard way to handle this is to grep the kernel source code
for that error message and then look at the code and figure out what
is wrong from that.
Unfortunately that is often far more difficult than needed because
with the strict 80 character limit printk format strings are often split up
over multiple lines. One example for this would be
printk(KERN_CRIT "protocol %04x is "
"buggy, dev %s\n",
skb2->protocol, dev->name);
from net/core/dev.c. If you see this obscure message what would you grep
for? "protocol.*is buggy" will not find it and "is buggy" neither and "buggy"
or "protocol" gives you a zillion of hits that you have review all
by hand. All quite unsatisfactory.
Worse the coding standard actually recommends to split up printks like
this, but I think it is one of the most counter0productive suggestions it
has.
There are two reasonable ways to handle this. Either put the format
string on an own line with backwards indentation or ignore the column 80
limitation. I think both are better actually than having ungreppable
error messages.
This patch updates the coding standard for to allow this.
Of course it only applies to the printk format string, any other
printk arguments should be still split up over multiple lines as before.
That is fine because they rarely need to be grepped for.
I expect this proposal will likely be somewhat controversal and I don't expect
universal agreement (and it's likely a bike shed paint topic like
all coding standard issues); but let's see what happens.
I think it would be a valuable improvement of the coding standard.
At some point checkpatch.pl would need to be updated to know about this
exception too, that would be the next step.
Signed-off-by: Andi Kleen <[email protected]>
Index: linux/Documentation/CodingStyle
===================================================================
--- linux.orig/Documentation/CodingStyle
+++ linux/Documentation/CodingStyle
@@ -83,20 +83,32 @@ preferred limit.
Statements longer than 80 columns will be broken into sensible chunks.
Descendants are always substantially shorter than the parent and are placed
substantially to the right. The same applies to function headers with a long
-argument list. Long strings are as well broken into shorter strings. The
-only exception to this is where exceeding 80 columns significantly increases
-readability and does not hide information.
+argument list.
-void fun(int a, int b, int c)
-{
- if (condition)
- printk(KERN_WARNING "Warning this is a long printk with "
- "3 parameters a: %u b: %u "
- "c: %u \n", a, b, c);
- else
- next_statement;
+It is not recommended to break printk format strings into smaller strings.
+The problem with doing this is that it makes it much harder to grep
+for the error messages in the source if they are split up over multiple
+lines. And grepping for error messages is fairly important for debugging.
+So for the special case of printk format strings (or formatting any other
+user visible error message) the normal 80 character column rule
+does not apply. Or alternatively it is ok to violate the indentation
+rule for the format string only if that makes the end not exceed
+80 characters. For example
+
+void function(void)
+{
+ if (...) {
+ if (...) {
+ printk(
+ "very very long formatting string with argument %d and argument %d\n",
+ a, b);
+ }
+ }
}
+You should still break up the further printk arguments if they exceed 80
+characters though.
+
Chapter 3: Placing Braces and Spaces
The other issue that always comes up in C styling is the placement of
Andi Kleen wrote:
> --- linux.orig/Documentation/CodingStyle
> +++ linux/Documentation/CodingStyle
> @@ -83,20 +83,32 @@ preferred limit.
> Statements longer than 80 columns will be broken into sensible chunks.
> Descendants are always substantially shorter than the parent and are placed
> substantially to the right. The same applies to function headers with a long
> -argument list. Long strings are as well broken into shorter strings. The
> -only exception to this is where exceeding 80 columns significantly increases
> -readability and does not hide information.
> +argument list.
>
> -void fun(int a, int b, int c)
> -{
> - if (condition)
> - printk(KERN_WARNING "Warning this is a long printk with "
> - "3 parameters a: %u b: %u "
> - "c: %u \n", a, b, c);
> - else
> - next_statement;
> +It is not recommended to break printk format strings into smaller strings.
Instead of a new recommendation (from now on we recommend something
contrary to what we required up until yesterday --- let's go unwrap
strings everywhere in the kernel now), how about simply saying that
printk format strings are not subject to the 80 column rule?
Or keep the old text and insert after "increases readability": "or
helps full-text searching". And delete the example code.
> +The problem with doing this is that it makes it much harder to grep
> +for the error messages in the source if they are split up over multiple
> +lines. And grepping for error messages is fairly important for debugging.
> +So for the special case of printk format strings (or formatting any other
> +user visible error message) the normal 80 character column rule
> +does not apply. Or alternatively it is ok to violate the indentation
> +rule for the format string only if that makes the end not exceed
> +80 characters. For example
> +
> +void function(void)
> +{
> + if (...) {
> + if (...) {
> + printk(
> + "very very long formatting string with argument %d and argument %d\n",
> + a, b);
> + }
> + }
> }
Here is one vote against this indentation exception.
PS: Could someone implement this for checkpatch.pl:
WARN_ON(hunk_is_in("Documentation/CodingStyle") &&
lines_added > lines_removed);
--
Stefan Richter
-=====-==--- --=- =-==-
http://arcgraph.de/sr/
> Instead of a new recommendation (from now on we recommend something
> contrary to what we required up until yesterday --- let's go unwrap
> strings everywhere in the kernel now), how about simply saying that
> printk format strings are not subject to the 80 column rule?
I personally slightly prefer the "unindent" solution over the ">80char"
solution, but it is not a very strong preference and both would be ok
for me.
-Andi
> At some point checkpatch.pl would need to be updated to know about this
> exception too, that would be the next step.
>
> Signed-off-by: Andi Kleen <[email protected]>
Acked-by: Alan Cox <[email protected]>
On Fri, 2008-02-22 at 14:26 +0100, Andi Kleen wrote:
> RFC: Update coding standard to avoid split up printk format strings
Perhaps it's more useful to have git become more content aware.
If git could track user specified file glob pattern ("*.[ch]$")
changes by statement terminator (";") and git could format
output by a user specified pass (lindent), many of the bike shed
paint discussions and a lot of commits would never occur.
Andi Kleen wrote:
> RFC: Update coding standard to avoid split up printk format strings
While we're talking about checkpatch.pl, I'd definitely like to teach
checkpatch about "list_for_each" and friends.
list_for_each is flow control, not a function call. I find it much
easier to see that something is a loop when there is a space between the
name and the parenthesis rather than when they are smashed together.
old patch follows
/Christer
checkpatch complains about the following:
WARNING: no space between function name and open parenthesis '('
#520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
+ list_for_each_entry (transfer, &message->transfers, transfer_list) {
which I think is a bit bogus since it actually is a for statement in
disguise. The following patch adds list_for_each to the list of things
that look like functions that it shouldn't complain about.
Index: linux-2.6.23/scripts/checkpatch.pl
===================================================================
--- linux-2.6.23.orig/scripts/checkpatch.pl
+++ linux-2.6.23/scripts/checkpatch.pl
@@ -681,7 +681,7 @@ sub process {
# check for spaces between functions and their parentheses.
if ($line =~ /($Ident)\s+\(/ &&
- $1 !~
/^(?:if|for|while|switch|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/
&&
+ $1 !~
/^(?:if|for|while|switch|list_for_each.*|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/
&&
$line !~ /$Type\s+\(/ && $line !~ /^.\#\s*define\b/) {
WARN("no space between function name and open parenthesis '('\n" .
$herecurr);
}
On Sat, Feb 23, 2008 at 01:55:32PM +0100, Christer Weinigel wrote:
> Andi Kleen wrote:
> >RFC: Update coding standard to avoid split up printk format strings
>
> While we're talking about checkpatch.pl, I'd definitely like to teach
> checkpatch about "list_for_each" and friends.
>
> list_for_each is flow control, not a function call. I find it much
> easier to see that something is a loop when there is a space between the
> name and the parenthesis rather than when they are smashed together.
>
> old patch follows
>
> /Christer
>
> checkpatch complains about the following:
>
> WARNING: no space between function name and open parenthesis '('
> #520: FILE: drivers/spi/spi_s3c24xx_dma.c:478:
> + list_for_each_entry (transfer, &message->transfers, transfer_list) {
>
> which I think is a bit bogus since it actually is a for statement in
> disguise. The following patch adds list_for_each to the list of things
> that look like functions that it shouldn't complain about.
>
> Index: linux-2.6.23/scripts/checkpatch.pl
> ===================================================================
> --- linux-2.6.23.orig/scripts/checkpatch.pl
> +++ linux-2.6.23/scripts/checkpatch.pl
> @@ -681,7 +681,7 @@ sub process {
>
> # check for spaces between functions and their parentheses.
> if ($line =~ /($Ident)\s+\(/ &&
> - $1 !~
> /^(?:if|for|while|switch|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/
> &&
> + $1 !~
> /^(?:if|for|while|switch|list_for_each.*|return|volatile|__volatile__|__attribute__|format|__extension__|Copyright)$/
> &&
> $line !~ /$Type\s+\(/ && $line !~ /^.\#\s*define\b/) {
> WARN("no space between function name and open
> parenthesis '('\n" . $herecurr);
> }
This has come up a few times. In previous discussions there were
supporters for spaces as well as not. Looking over the existing in kernel
use, the majority are without a space (from before checkpatch encouraged
that usage); and so it is that that won the day.
-apw
On Fri, Feb 22, 2008 at 02:26:12PM +0100, Andi Kleen wrote:
> At some point checkpatch.pl would need to be updated to know about this
> exception too, that would be the next step.
Cirtainly we have exceptions for docstrings, so that shouldn't be a
problem if this were accepted.
-apw