[PATCH] CodingStyle: Printing numbers in parentheses is fine

[PATCH] CodingStyle: Printing numbers in parentheses is fine

[Jean Delvare]

Remove a not particularly relevant rule from CodingStyle.
Sometimes, printing numbers in parentheses doesn't add value, but in
some (most?) cases it makes the message easier to read. As a matter of
fact, this practice is widely used in the kernel:

linux-2.6.23-rc8$ quilt grep -I '(%l*[du])' | wc -l
3166
linux-2.6.23-rc8$

Signed-off-by: Jean Delvare <khali@linux-fr.org>
---
 Documentation/CodingStyle |    2 --
 1 file changed, 2 deletions(-)

--- linux-2.6.23-rc8.orig/Documentation/CodingStyle	2007-07-23 16:44:32.000000000 +0200
+++ linux-2.6.23-rc8/Documentation/CodingStyle	2007-09-28 23:53:23.000000000 +0200
@@ -638,8 +638,6 @@ concise, clear, and unambiguous.
 
 Kernel messages do not have to be terminated with a period.
 
-Printing numbers in parentheses (%d) adds no value and should be avoided.
-
 There are a number of driver model diagnostic macros in <linux/device.h>
 which you should use to make sure messages are matched to the right device
 and driver, and are tagged with the right level:  dev_err(), dev_warn(),


-- 
Jean Delvare

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Andrew Morton]

On Sat, 29 Sep 2007 12:25:30 +0200 Jean Delvare <khali@linux-fr.org> wrote:

> Remove a not particularly relevant rule from CodingStyle.
> Sometimes, printing numbers in parentheses doesn't add value, but in
> some (most?) cases it makes the message easier to read. As a matter of
> fact, this practice is widely used in the kernel:
> 
> linux-2.6.23-rc8$ quilt grep -I '(%l*[du])' | wc -l
> 3166
> linux-2.6.23-rc8$
> 
> Signed-off-by: Jean Delvare <khali@linux-fr.org>
> ---
>  Documentation/CodingStyle |    2 --
>  1 file changed, 2 deletions(-)
> 
> --- linux-2.6.23-rc8.orig/Documentation/CodingStyle	2007-07-23 16:44:32.000000000 +0200
> +++ linux-2.6.23-rc8/Documentation/CodingStyle	2007-09-28 23:53:23.000000000 +0200
> @@ -638,8 +638,6 @@ concise, clear, and unambiguous.
>  
>  Kernel messages do not have to be terminated with a period.
>  
> -Printing numbers in parentheses (%d) adds no value and should be avoided.
> -
>  There are a number of driver model diagnostic macros in <linux/device.h>
>  which you should use to make sure messages are matched to the right device
>  and driver, and are tagged with the right level:  dev_err(), dev_warn(),

I wonder how that got there.

Printing something like

	bytes remaining: 0x12 (18)

is a quite logical thing to do, although pretty darm pointless.


otoh, looking at the various instances, we have lots of stuff like this:


  printk(KERN_ERR "seq-oss: unable to delete queue %d (%d)\n", queue, rc);

which I would argue is wrong and is inconsistent with most other error
reporting.  It should be

	unable to delete queue %d: %d



And this:

	printk(KERN_ERR "%s:  context size (%u) exceeds payload "

doesn't need the parens


Here:

	printk("hardirqs last  enabled at (%u): ", curr->hardirq_enable_event);
	print_ip_sym(curr->hardirq_enable_ip);
	printk("hardirqs last disabled at (%u): ", curr->hardirq_disable_event);
	print_ip_sym(curr->hardirq_disable_ip);
	printk("softirqs last  enabled at (%u): ", curr->softirq_enable_event);
	print_ip_sym(curr->softirq_enable_ip);
	printk("softirqs last disabled at (%u): ", curr->softirq_disable_event);
	print_ip_sym(curr->softirq_disable_ip);

all the parens are just illogical and should be removed.


Here:

		xlog_warn("XFS: %s: unrecognised log version (%d).",
			__FUNCTION__, INT_GET(rhead->h_version, ARCH_CONVERT));

should use "unrecognised log version: %d"


This:

		printk(KERN_ERR "udf: unknown compression code (%d) stri=%s\n",
		       cmp_id, ocu_i->u_name);

should use colon as well.


So in fact a large number of the instances I see in there are illogical and
basically gramatically wrong and should be converted to use a colon.

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[David Brownell]

> > -Printing numbers in parentheses (%d) adds no value and should be avoided.
>
> I wonder how that got there.

Well, the only place that numbers "naturally" appear wrapped in
parenthesis is tables of credits and debits... as a debit, sort
of a literal "add no value" situation.  ;)

Oh, and for footnotes, in typographically challenged contexts.

Me, I agree that numbers in parens don't usually make sense for
kernel messages.

- Dave

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Randy Dunlap]

On Sat, 29 Sep 2007 03:51:56 -0700 Andrew Morton wrote:

> On Sat, 29 Sep 2007 12:25:30 +0200 Jean Delvare <khali@linux-fr.org> wrote:
> 
> > Remove a not particularly relevant rule from CodingStyle.
> > Sometimes, printing numbers in parentheses doesn't add value, but in
> > some (most?) cases it makes the message easier to read. As a matter of
> > fact, this practice is widely used in the kernel:
> > 
> > linux-2.6.23-rc8$ quilt grep -I '(%l*[du])' | wc -l
> > 3166
> > linux-2.6.23-rc8$
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > ---
> >  Documentation/CodingStyle |    2 --
> >  1 file changed, 2 deletions(-)
> > 
> > --- linux-2.6.23-rc8.orig/Documentation/CodingStyle	2007-07-23 16:44:32.000000000 +0200
> > +++ linux-2.6.23-rc8/Documentation/CodingStyle	2007-09-28 23:53:23.000000000 +0200
> > @@ -638,8 +638,6 @@ concise, clear, and unambiguous.
> >  
> >  Kernel messages do not have to be terminated with a period.
> >  
> > -Printing numbers in parentheses (%d) adds no value and should be avoided.
> > -
> >  There are a number of driver model diagnostic macros in <linux/device.h>
> >  which you should use to make sure messages are matched to the right device
> >  and driver, and are tagged with the right level:  dev_err(), dev_warn(),
> 
> I wonder how that got there.

http://linux.bkbits.net:8080/linux-2.6/?PAGE=cset&REV=4034429a6JsOCMNXT3tTPAX1kX40bg

Let's kill it, please.  (i.e., ACK)

---
~Randy

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[David Brownell]

> From randy.dunlap@oracle.com  Sat Sep 29 11:40:17 2007
>
> Let's kill it, please.  (i.e., ACK)

But ... why?  What value could needless parens provide?
"Yet Another Subtle And Hard To Fix Source Of Bloat" is
not a plus.

I'd kind of think a change like this should have some
positive motivation.

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Randy Dunlap]

On Sat, 29 Sep 2007 11:53:06 -0700 David Brownell wrote:

> > From randy.dunlap@oracle.com  Sat Sep 29 11:40:17 2007
> >
> > Let's kill it, please.  (i.e., ACK)
> 
> But ... why?  What value could needless parens provide?

Who says that needless parens could provide value?

> "Yet Another Subtle And Hard To Fix Source Of Bloat" is
> not a plus.

ISTM that we agree.

> I'd kind of think a change like this should have some
> positive motivation.

"Me, I agree that numbers in parens don't usually make sense for
kernel messages."

It's too trivial to be included in CodingStyle.

---
~Randy

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[David Brownell]

> > > Let's kill it, please.  (i.e., ACK)
> > 
> > But ... why?  What value could needless parens provide?
>
> Who says that needless parens could provide value?

Jean, which is why he submitted the patch.
You, implicitly, by acking a patch saying those parens are bad.
But not me ... I don't think this patch is merge-worthy.


> > "Yet Another Subtle And Hard To Fix Source Of Bloat" is
> > not a plus.
>
> ISTM that we agree.

Evidently not, since removing it would promote such bloat.
Explicitly removing disapproval is a form of approval...


> > I'd kind of think a change like this should have some
> > positive motivation.
>
> "Me, I agree that numbers in parens don't usually make sense for
> kernel messages."
>
> It's too trivial to be included in CodingStyle.

So the reason to remove that guideline, and thereby encourage
proliferation of needless parens, is ... that some OTHER way
to get rid of them is working?

I would agree that line could be improved to say that messages
should not be needlessly large.  Excess parens are one example;
wordiness is another (including printing 8 bit fields as 0x%08x),
and I'm sure there are more.

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Randy Dunlap]

On Sat, 29 Sep 2007 15:30:15 -0700 David Brownell wrote:

> > > > Let's kill it, please.  (i.e., ACK)
> > > 
> > > But ... why?  What value could needless parens provide?
> >
> > Who says that needless parens could provide value?
> 
> Jean, which is why he submitted the patch.
> You, implicitly, by acking a patch saying those parens are bad.
> But not me ... I don't think this patch is merge-worthy.

Thanks for clearing that up.  Yes, you did have me confused.

Sure, if something is needless, it doesn't provide value.
So we disagree that some parens are needless.  OK.

> > > "Yet Another Subtle And Hard To Fix Source Of Bloat" is
> > > not a plus.
> >
> > ISTM that we agree.
> 
> Evidently not, since removing it would promote such bloat.
> Explicitly removing disapproval is a form of approval...
> 
> 
> > > I'd kind of think a change like this should have some
> > > positive motivation.
> >
> > "Me, I agree that numbers in parens don't usually make sense for
> > kernel messages."
> >
> > It's too trivial to be included in CodingStyle.
> 
> So the reason to remove that guideline, and thereby encourage
> proliferation of needless parens, is ... that some OTHER way
> to get rid of them is working?

Andrew listed some cases where parens make sense.  He didn't say
(and I don't say) [oops, parens] that they always make sense,
but the statement in CodingStyle is too strict.  Sometimes they
make sense.

> I would agree that line could be improved to say that messages
> should not be needlessly large.  Excess parens are one example;
> wordiness is another (including printing 8 bit fields as 0x%08x),
> and I'm sure there are more.

---
~Randy

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Måns Rullgård]

David Brownell <david-b@pacbell.net> writes:

>> > > Let's kill it, please.  (i.e., ACK)
>> > 
>> > But ... why?  What value could needless parens provide?
>>
>> Who says that needless parens could provide value?
>
> Jean, which is why he submitted the patch.
> You, implicitly, by acking a patch saying those parens are bad.
> But not me ... I don't think this patch is merge-worthy.

Would also add rules like "don't put parens around the word device"
etc?  There are countless silly things one could do, and we can't
explicitly prohibit all of them.

-- 
Måns Rullgård
mans@mansr.com

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Jean Delvare]

Hi David,

On Sat, 29 Sep 2007 15:30:15 -0700, David Brownell wrote:
> > > > Let's kill it, please.  (i.e., ACK)
> > > 
> > > But ... why?  What value could needless parens provide?

By definition of "needless", none. But the question is precisely
whether the parentheses are always needless. The way your formulate
your question suggests that you aren't actually interested in
discussing the matter, so I am wondering why are you taking part at all.

> > Who says that needless parens could provide value?
> 
> Jean, which is why he submitted the patch.

No, I didn't. I said that parentheses were sometimes useful. You said
that parentheses were usually needless (which isn't that different,
BTW.) Combining both statements into "useless parentheses provide
value" is a logical flaw. And attributing it to me is dishonest.

> (...)
> > > I'd kind of think a change like this should have some
> > > positive motivation.

My "positive motivation" was that this statement, as is, is more
confusing than helpful. As a matter of fact, it led us to disagree on a
patch you sent. And I would like contributors to focus on the more
important parts of CodingStyle. If someone wants to improve the
statement instead of removing it, that's fine with me, but not having
the time to work on this, I proposed the removal.

> I would agree that line could be improved to say that messages
> should not be needlessly large.

It does already:

"Make the messages concise, clear, and unambiguous."

-- 
Jean Delvare

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Sat, 29 Sep 2007 03:51:56 PDT, Andrew Morton said:

> Printing something like
> 
> 	bytes remaining: 0x12 (18)
> 
> is a quite logical thing to do, although pretty darm pointless.

On the other hand, printing this:

        magic number: 0x2710

probably doesn't ring any bells, but if you changed that format to be

	"magic number: %x (%d)",foo,foo

you'll almost certainly sit up and ask "Where the fsck did *that* come from?"

(unless you're a *lot* better at doing hex conversions in your head than I).

Yeah, *usually* pointless, but it has its place sometimes....


[-- Attachment #2: Type: application/pgp-signature, Size: 226 bytes --]

Re: [PATCH] CodingStyle: Printing numbers in parentheses is fine

[Jean Delvare]

Hi Andrew,

On Sat, 29 Sep 2007 03:51:56 -0700, Andrew Morton wrote:
> On Sat, 29 Sep 2007 12:25:30 +0200 Jean Delvare <khali@linux-fr.org> wrote:
> 
> > Remove a not particularly relevant rule from CodingStyle.
> > Sometimes, printing numbers in parentheses doesn't add value, but in
> > some (most?) cases it makes the message easier to read. As a matter of
> > fact, this practice is widely used in the kernel:
> > 
> > linux-2.6.23-rc8$ quilt grep -I '(%l*[du])' | wc -l
> > 3166
> > linux-2.6.23-rc8$
> > 
> > Signed-off-by: Jean Delvare <khali@linux-fr.org>
> > ---
> >  Documentation/CodingStyle |    2 --
> >  1 file changed, 2 deletions(-)
> > 
> > --- linux-2.6.23-rc8.orig/Documentation/CodingStyle	2007-07-23 16:44:32.000000000 +0200
> > +++ linux-2.6.23-rc8/Documentation/CodingStyle	2007-09-28 23:53:23.000000000 +0200
> > @@ -638,8 +638,6 @@ concise, clear, and unambiguous.
> >  
> >  Kernel messages do not have to be terminated with a period.
> >  
> > -Printing numbers in parentheses (%d) adds no value and should be avoided.
> > -
> >  There are a number of driver model diagnostic macros in <linux/device.h>
> >  which you should use to make sure messages are matched to the right device
> >  and driver, and are tagged with the right level:  dev_err(), dev_warn(),
> 
> I wonder how that got there.
> 
> Printing something like
> 
> 	bytes remaining: 0x12 (18)
> 
> is a quite logical thing to do, although pretty darm pointless.
> 
> 
> otoh, looking at the various instances, we have lots of stuff like this:
> 
> 
>   printk(KERN_ERR "seq-oss: unable to delete queue %d (%d)\n", queue, rc);
> 
> which I would argue is wrong and is inconsistent with most other error
> reporting.  It should be
> 
> 	unable to delete queue %d: %d

I disagree. Reporting an error code is precisely the case where I think
(%d) is valuable. A colon suggests that an understandable explanation
follows, while an error code isn't that understandable. What I expect
as a user is:

	Read failed: I/O error

but:

	Read failed (-5)

And I thought this was the usual convention, too, but you seem to
disagree.

> And this:
> 
> 	printk(KERN_ERR "%s:  context size (%u) exceeds payload "
> 
> doesn't need the parens

Agreed.

> Here:
> 
> 	printk("hardirqs last  enabled at (%u): ", curr->hardirq_enable_event);
> 	print_ip_sym(curr->hardirq_enable_ip);
> 	printk("hardirqs last disabled at (%u): ", curr->hardirq_disable_event);
> 	print_ip_sym(curr->hardirq_disable_ip);
> 	printk("softirqs last  enabled at (%u): ", curr->softirq_enable_event);
> 	print_ip_sym(curr->softirq_enable_ip);
> 	printk("softirqs last disabled at (%u): ", curr->softirq_disable_event);
> 	print_ip_sym(curr->softirq_disable_ip);
> 
> all the parens are just illogical and should be removed.

Agreed.

> Here:
> 
> 		xlog_warn("XFS: %s: unrecognised log version (%d).",
> 			__FUNCTION__, INT_GET(rhead->h_version, ARCH_CONVERT));
> 
> should use "unrecognised log version: %d"

I'm fine both ways.

> This:
> 
> 		printk(KERN_ERR "udf: unknown compression code (%d) stri=%s\n",
> 		       cmp_id, ocu_i->u_name);
> 
> should use colon as well.
> 
> 
> So in fact a large number of the instances I see in there are illogical and
> basically gramatically wrong and should be converted to use a colon.

I would be fine having one or more explicit rules in CodingStyle on how
log messages should be formatted, if you think it adds value. But a
general statement "don't do foo" when it's sometimes just fine to do
foo, doesn't add any value IMHO.

-- 
Jean Delvare