CodingStyle: Printing numbers in parentheses is fine
PATCH 1/1 CodingStyle: Printing numbers in parentheses is fine
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$
--- Documentation/CodingStyle | 2 -- 1 file changed, 2 deletions(-)
Documentation/CodingStyle +0 -2
@@ -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(),
Signature · 1 line
Jean Delvare
Quoted 17 lines · On Sat, 29 Sep 2007 12:25:30 +0200 Jean Delvare <[email protected]> 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 <[email protected]>--- Documentation/CodingStyle | 2 -- 1 file changed, 2 deletions(-)
Documentation/CodingStyle concise, clear, and unambiguous. · line 643
Quoted 8 lines
Documentation/CodingStyle +0
-2@@ -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.
Quoted 3 lines
Quoted 1 line
-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
Quoted 22 lines · On Sat, 29 Sep 2007 03:51:56 -0700 Andrew Morton wrote:
Quoted 20 lines · On Sat, 29 Sep 2007 12:25:30 +0200 Jean Delvare <[email protected]> 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 <[email protected]>--- Documentation/CodingStyle | 2 -- 1 file changed, 2 deletions(-)Documentation/CodingStyle +0
-0@@ -638,8 +638,6 @@ concise, clear, and unambiguous. Kernel messages do not have to be terminated with a period.
Quoted 7 lines
Documentation/CodingStyle concise, clear, and unambiguous. · line 643
Quoted 5 lines
Documentation/CodingStyle +0
-2-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
Quoted 3 lines
From [email protected] 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.
Quoted 5 lines · On Sat, 29 Sep 2007 11:53:06 -0700 David Brownell wrote:
Quoted 3 lines
From [email protected] 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?
Quoted 2 lines
"Yet Another Subtle And Hard To Fix Source Of Bloat" is not a plus.
ISTM that we agree.
Quoted 2 lines
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
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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.
Quoted 4 lines
Quoted 2 lines
"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...
Quoted 7 lines
Quoted 2 lines
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.
Quoted 9 lines · On Sat, 29 Sep 2007 15:30:15 -0700 David Brownell wrote:
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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.
Quoted 12 lines
Quoted 4 lines
Quoted 2 lines
"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...
Quoted 3 lines
Quoted 2 lines
I'd kind of think a change like this should have some positive motivation.
Quoted 8 lines
Quoted 4 lines
"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.
Quoted 4 lines
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
Quoted 9 lines · David Brownell
Quoted 5 lines
Quoted 3 lines
Quoted 1 line
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.
Signature · 2 lines
Måns Rullgård [email protected]
Hi David,
Quoted 3 lines · David Brownell
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
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.
Quoted 3 lines
Quoted 1 line
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.
Quoted 3 lines
(...)
Quoted 2 lines
Quoted 2 lines
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.
Quoted 2 lines
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."
Signature · 1 line
Jean Delvare
Quoted 5 lines · Andrew Morton
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....
Hi Andrew,
Quoted 40 lines · Andrew Morton
Quoted 17 lines · On Sat, 29 Sep 2007 12:25:30 +0200 Jean Delvare <[email protected]> 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 <[email protected]>--- Documentation/CodingStyle | 2 -- 1 file changed, 2 deletions(-)Documentation/CodingStyle concise, clear, and unambiguous. · line 643
Quoted 8 lines
Documentation/CodingStyle +0
-2@@ -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:
Quoted 6 lines
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.
Quoted 5 lines
And this:
printk(KERN_ERR "%s: context size (%u) exceeds payload "
doesn't need the parens
Agreed.
Quoted 11 lines
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);
Quoted 1 line
all the parens are just illogical and should be removed.
Agreed.
Quoted 6 lines
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.
Quoted 10 lines
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.
Signature · 1 line
Jean Delvare