Documentation/CodingStyle: Add rules for goto labels
RFC 1/1 Documentation/CodingStyle: Add rules for goto labels
There are many people use Lindent to indent their code, but Lindent gets wrong when it meets goto labels. This should be fixed manually.
And kernel's CodingStyle doesn't specify the indentation for goto labels explicitly.
This patch adds specifications on those things about goto labels in CodingStyle. And it is against -rc3 source tree.
Any advice or comments are more than welcome!
Regards!
--- CodingStyle | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/CodingStyle renamed from Documentation/CodingStyle.orig +6 -0
@@ -66,6 +66,12 @@ something to hide: Don't put multiple assignments on a single line either. Kernel coding style is super simple. Avoid tricky expressions. +Do care when you use Lindent to indent your code, since it may use spaces +instead of tabs before a goto label and it may also align the label in a +wrong position. A goto label should be aligned in the column that is 8 +characters ahead of the statement just below this label. Please fix it manually +if you find Lindent is wrong. + Outside of comments, documentation and except in Kconfig, spaces are never used for indentation, and the above example is deliberately broken.
Documentation/CodingStyle something to hide: · line 73
Quoted 5 lines · WANG Cong
Documentation/CodingStyle renamed from Documentation/CodingStyle.orig +5
-0+Do care when you use Lindent to indent your code, since it may use spaces +instead of tabs before a goto label and it may also align the label in a +wrong position. A goto label should be aligned in the column that is 8 +characters ahead of the statement just below this label. Please fix it manually +if you find Lindent is wrong.
Lindent is wrong, but the style you are advocating is, at the very least, not universal. Equally (if not more) common is putting label in column 1, period. Regardless of indentation level of the statement following it.
Quoted 12 lines · On Sun, 3 Jun 2007 15:29:39 +0100 Al Viro wrote:
Documentation/CodingStyle something to hide: · line 73
Quoted 5 lines · WANG Cong
Documentation/CodingStyle renamed from Documentation/CodingStyle.orig +5
-0+Do care when you use Lindent to indent your code, since it may use spaces +instead of tabs before a goto label and it may also align the label in a +wrong position. A goto label should be aligned in the column that is 8 +characters ahead of the statement just below this label. Please fix it manually +if you find Lindent is wrong.Lindent is wrong, but the style you are advocating is, at the very least, not universal. Equally (if not more) common is putting label in column 1, period. Regardless of indentation level of the statement following it.
ack Al's style.
---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
Quoted 14 lines · Randy Dunlap
Quoted 11 lines · On Sun, 3 Jun 2007 15:29:39 +0100 Al Viro wrote:
Documentation/CodingStyle something to hide: · line 73
Quoted 5 lines · WANG Cong
Documentation/CodingStyle renamed from Documentation/CodingStyle.orig +5
-0+Do care when you use Lindent to indent your code, since it may use spaces +instead of tabs before a goto label and it may also align the label in a +wrong position. A goto label should be aligned in the column that is 8 +characters ahead of the statement just below this label. Please fix it manually +if you find Lindent is wrong.Lindent is wrong, but the style you are advocating is, at the very least, not universal. Equally (if not more) common is putting label in column 1, period. Regardless of indentation level of the statement following it.
Quoted 1 line
ack Al's style.
Seconded. All my code contains the goto label in the first column.
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
Jeff
Quoted 17 lines · Jeff Garzik
Quoted 16 lines · Randy Dunlap
Quoted 13 lines · On Sun, 3 Jun 2007 15:29:39 +0100 Al Viro wrote:
Quoted 7 lines · WANG Cong
+Do care when you use Lindent to indent your code, since it may use spaces
Documentation/CodingStyle renamed from Documentation/CodingStyle.orig +3
-0+instead of tabs before a goto label and it may also align the label in a +wrong position. A goto label should be aligned in the column that is 8 +characters ahead of the statement just below this label. Please fix itmanually
+if you find Lindent is wrong.Lindent is wrong, but the style you are advocating is, at the very least, not universal. Equally (if not more) common is putting label in column 1, period. Regardless of indentation level of the statement following it.
Quoted 8 lines
Quoted 1 line
ack Al's style.
Seconded. All my code contains the goto label in the first column.
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
Jeff
Thanks for all comments!
I just wonder, if a goto label is nested in a while/for/if/switch block, aligning it in the first column maybe a bit ugly. (I know mostly it is not in any while/for/if/switch block.)
Regards!
WANG Cong
Quoted 3 lines · WANG Cong
I just wonder, if a goto label is nested in a while/for/if/switch block, aligning it in the first column maybe a bit ugly. (I know mostly it is not in any while/for/if/switch block.)
In general goto labels are not scoped, so there's no point in pretending they are. It might make sense to indent a label deeper if you've actually declared it local (__label__).
J
Quoted 10 lines · Jeremy Fitzhardinge
Quoted 3 lines · WANG Cong
I just wonder, if a goto label is nested in a while/for/if/switch block, aligning it in the first column maybe a bit ugly. (I know mostly it is not in any while/for/if/switch block.)
In general goto labels are not scoped, so there's no point in pretending they are. It might make sense to indent a label deeper if you've actually declared it local (
__label__).J
OK. Thank you. I will take your advice.
A new version of this patch will come soon. ;)
RFC 1/1 Documentation/CodingStyle: Add rules for goto labels (-v2)
In reply to Jeremy FitzhardingeLindent gets wrong when it meets goto labels. This should be fixed manually. And kernel's CodingStyle doesn't specify the indentation for goto labels explicitly.
This patch adds specifications on those things about goto labels in CodingStyle. And it is against -rc3 source tree.
Thanks to Al Viro, Randy Dunlap, Jeff Garzik and Jeremy Fitzhardinge for their comments!
Regards!
--- CodingStyle | 7 +++++++ 1 file changed, 7 insertions(+)
Documentation/CodingStyle renamed from Documentation/CodingStyle.orig +7 -0
@@ -66,6 +66,13 @@ something to hide: Don't put multiple assignments on a single line either. Kernel coding style is super simple. Avoid tricky expressions. +Do care when you use Lindent to indent your code, since it may use spaces +instead of tabs before a goto label and it may also align the label in a +wrong position. Generally speaking, a goto label should be always aligned in +the first column. However, it might make sense to indent a label deeper if +you've actually declared it local (__label__). Please fix it manually if you +find Lindent is wrong. + Outside of comments, documentation and except in Kconfig, spaces are never used for indentation, and the above example is deliberately broken.
Re: [RFC] Documentation/CodingStyle: Add rules for goto labels (-v2)
Quoted 20 lines · WANG Cong
Lindent gets wrong when it meets goto labels. This should be fixed manually. And kernel's CodingStyle doesn't specify the indentation for goto labels explicitly.
This patch adds specifications on those things about goto labels in CodingStyle. And it is against -rc3 source tree.
Thanks to Al Viro, Randy Dunlap, Jeff Garzik and Jeremy Fitzhardinge for their comments!
Regards!
Signed-off-by WANG Cong <[email protected]>--- CodingStyle | 7 +++++++ 1 file changed, 7 insertions(+)
Documentation/CodingStyle something to hide: · line 74
Quoted 9 lines
Documentation/CodingStyle renamed from Documentation/CodingStyle.orig +6
-0@@ -66,6 +66,13 @@ something to hide: Don't put multiple assignments on a single line either. Kernel coding style is super simple. Avoid tricky expressions. +Do care when you use Lindent to indent your code, since it may use spaces +instead of tabs before a goto label and it may also align the label in a +wrong position. Generally speaking, a goto label should be always aligned in +the first column. However, it might make sense to indent a label deeper if +you've actually declared it local (__label__). Please fix it manually if you +find Lindent is wrong.
If local(__label__) really so widely used in the kernel that it deserves a place in coding-style?
A quick grep did not say so.
Sam
Re: [RFC] Documentation/CodingStyle: Add rules for goto labels (-v2)
Quoted 4 lines · Sam Ravnborg
If
local(__label__)really so widely used in the kernel that it deserves a place in coding-style?
A quick grep did not say so.
Probably not, and its use shouldn't be (even tacitly) encouraged. It's only really useful for defining a local label within a macro, and there's probably something wrong with your macro if you need a local label.
J
Re: [RFC] Documentation/CodingStyle: Add rules for goto labels (-v2)
Quoted 11 lines · Jeremy Fitzhardinge
Quoted 4 lines · Sam Ravnborg
If
local(__label__)really so widely used in the kernel that it deserves a place in coding-style?
A quick grep did not say so.Probably not, and its use shouldn't be (even tacitly) encouraged. It's only really useful for defining a local label within a macro, and there's probably something wrong with your macro if you need a local label.
J
I agree. Local labels maybe useful in macros.
If someone will use local labels in his code some day, we shouldn't force him to align his labels in the first column.
Re: [RFC] Documentation/CodingStyle: Add rules for goto labels (-v2)
Quoted 3 lines · WANG Cong
I agree. Local labels maybe useful in macros.
If someone will use local labels in his code some day, we shouldn't force him to align his labels in the first column.
No, we should just force him to rewrite his dreck into a readable form and get rid of that junk...
Quoted 8 lines · Jeremy Fitzhardinge
Quoted 2 lines · WANG Cong
I just wonder, if a goto label is nested in a while/for/if/switch block, aligning it in the first column maybe a bit ugly. (I know mostly it is not in any while/for/if/switch block.)
In general goto labels are not scoped, so there's no point in pretending they are. It might make sense to indent a label deeper if you've actually declared it local (
__label__).
goto labels are scoped in one sense: they are only reachable from inside the block they are defined in, so I would disagree with this statement.
-hpa
Quoted 3 lines · H. Peter Anvin
goto labels are scoped in one sense: they are only reachable from inside the block they are defined in, so I would disagree with this statement.
Erm, no I think you're wrong there (or we're talking at cross purposes). They're visible to the whole function they're defined in, regardless of what block scope happens to be current at the time. You need to use the gcc "__label__" extension to make a locally-scoped label. Otherwise this wouldn't work:
if (a_failed)
goto cleanup;
[...]
if (b_failed) {
cleanup:
cleanup();
}
JQuoted 6 lines · Jeremy Fitzhardinge
Quoted 3 lines · H. Peter Anvin
goto labels are scoped in one sense: they are only reachable from inside the block they are defined in, so I would disagree with this statement.
Erm, no I think you're wrong there
Yeah, I realized I was talking out of my ass there when Viro pointed this out. I has inadvertently generalized the rule "no skipping automatics" to "no jump into blocks."
-hpa
Quoted 6 lines · Jeff Garzik
Seconded. All my code contains the goto label in the first column.
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
I would have to disagree with this. IMNSHO, a goto label is like a case label, and they should be treated the same way.
-hpa
Quoted 9 lines · H. Peter Anvin
Quoted 5 lines · Jeff Garzik
Seconded. All my code contains the goto label in the first column.
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
I would have to disagree with this. IMNSHO, a goto label is like a case label, and they should be treated the same way.
But gotos are special. ("Evil" minus the "it's good for unrolling in case of an error" case).
Jan
Quoted 4 lines · Jan Engelhardt
Quoted 3 lines · H. Peter Anvin
Quoted 2 lines · Jeff Garzik
Seconded. All my code contains the goto label in the first column.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
I would have to disagree with this. IMNSHO, a goto label is like a case label, and they should be treated the same way.
But gotos are special. ("Evil" minus the "it's good for unrolling in case of an error" case).
So?
You still want them to be associated with the level the bailout happens at.
-hpa
Quoted 7 lines · H. Peter Anvin
Quoted 2 lines · Jan Engelhardt
But gotos are special. ("Evil" minus the "it's good for unrolling in case of an error" case).
So?
You still want them to be associated with the level the bailout happens at.
No, you want them associated with the level you bail out *TO*. Otherwise you get this:
...
...
if (..) {
if (..) {
goto ka_blammo;
...
foo = yadda_yadda(.....);
kfree(bar->some_struct);
ka_blammo:
/* continue with cleanup here */Goto labels belong in column 1. Or 2 if you believe in the existence of 'diff -p' that get confused by col-1 labels.
Quoted 6 lines · [email protected] wrote:
Quoted 2 lines
You still want them to be associated with the level the bailout happens at.
No, you want them associated with the level you bail out *TO*. Otherwise you get this:
Right, I misspoke.
-hpa
Quoted 10 lines · H. Peter Anvin
Quoted 9 lines · Jan Engelhardt
Quoted 7 lines · H. Peter Anvin
Quoted 4 lines · Jeff Garzik
Seconded. All my code contains the goto label in the first column.
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
I would have to disagree with this. IMNSHO, a goto label is like a case label, and they should be treated the same way.
Quoted 7 lines
Quoted 3 lines
But gotos are special. ("Evil" minus the "it's good for unrolling in case of an error" case).
So?
You still want them to be associated with the level the bailout happens at.
A matter of opinion :) I tend to think goto is special enough to warrant column 1 unconditionally. It is special, so it draws additional attention over and above case labels.
I and others have been tripped up when programmers "hide" goto statements among regular statements.
IMO goto warrants a big flashing "notice me" sign.
Jeff
Quoted 23 lines · Jeff Garzik
Quoted 10 lines · H. Peter Anvin
Quoted 9 lines · Jan Engelhardt
Quoted 7 lines · H. Peter Anvin
Quoted 4 lines · Jeff Garzik
Seconded. All my code contains the goto label in the first column.
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
I would have to disagree with this. IMNSHO, a goto label is like a case label, and they should be treated the same way.
Quoted 7 lines
Quoted 3 lines
But gotos are special. ("Evil" minus the "it's good for unrolling in case of an error" case).
So?
You still want them to be associated with the level the bailout happens at.
A matter of opinion :) I tend to think goto is special enough to warrant column 1 unconditionally. It is special, so it draws additional attention over and above case labels.
Quoted 6 lines
I and others have been tripped up when programmers "hide" goto statements among regular statements.
IMO goto warrants a big flashing "notice me" sign.
Jeff
Hmmm, perhaps.
So, it seems that we can reach an agreement. Any other comments or suggestions? Or can someone ack/merge this patch?
Thanks!
Quoted 27 lines · WANG Cong
Quoted 26 lines · Jeff Garzik
Quoted 10 lines · H. Peter Anvin
Quoted 9 lines · Jan Engelhardt
Quoted 7 lines · H. Peter Anvin
Quoted 4 lines · Jeff Garzik
Seconded. All my code contains the goto label in the first column.
IMO any other goto label indentation is silly, because it obscures the goto label within the code block.
I would have to disagree with this. IMNSHO, a goto label is like a case label, and they should be treated the same way.
Quoted 7 lines
Quoted 3 lines
But gotos are special. ("Evil" minus the "it's good for unrolling in case of an error" case).
So?
You still want them to be associated with the level the bailout happens at.
A matter of opinion :) I tend to think goto is special enough to warrant column 1 unconditionally. It is special, so it draws additional attention over and above case labels.
I and others have been tripped up when programmers "hide" goto statements among regular statements.
Quoted 8 lines
Quoted 3 lines
IMO goto warrants a big flashing "notice me" sign.
Jeff
Hmmm, perhaps.
So, it seems that we can reach an agreement. Any other comments or suggestions? Or can someone ack/merge this patch?
Honestly, I think not reaching an agreement is a good thing.
"style" is always ultimately in the eye of the beholder, and reasoned people come up with multiple answers to the same question.
Sometimes it's better _not_ to set things in stone. "use your judgement" and "follow the existing style" go a long way.
Jeff
Quoted 4 lines · Jeff Garzik
Quoted 3 lines
So, it seems that we can reach an agreement. Any other comments or suggestions? Or can someone ack/merge this patch?
Quoted 8 lines
Honestly, I think not reaching an agreement is a good thing.
"style" is always ultimately in the eye of the beholder, and reasoned people come up with multiple answers to the same question.
Sometimes it's better _not_ to set things in stone. "use your judgement" and "follow the existing style" go a long way.
Indeed. Quite frankly, style guides would be a lot less obnoxious if there weren't always people who felt the need to "correct" other people's styles. That is usually a rather obnoxious thing to do, at least as long as the code is reasonably clean to start out with.
-hpa
Quoted 10 lines · H. Peter Anvin
Quoted 9 lines · Jeff Garzik
Quoted 3 lines
So, it seems that we can reach an agreement. Any other comments or suggestions? Or can someone ack/merge this patch?
Honestly, I think not reaching an agreement is a good thing.
"style" is always ultimately in the eye of the beholder, and reasoned people come up with multiple answers to the same question.
Quoted 9 lines
Quoted 3 lines
Sometimes it's better _not_ to set things in stone. "use your judgement" and "follow the existing style" go a long way.
Indeed. Quite frankly, style guides would be a lot less obnoxious if there weren't always people who felt the need to "correct" other people's styles. That is usually a rather obnoxious thing to do, at least as long as the code is reasonably clean to start out with.
I find things about coding style can easily lead to flamewars anywhere.
Maybe this patch is considered harmful as goto statements once were. ;-p
Quoted 9 lines · WANG Cong
Quoted 7 lines · Jeff Garzik
A matter of opinion :) I tend to think goto is special enough to warrant column 1 unconditionally. It is special, so it draws additional attention over and above case labels.
I and others have been tripped up when programmers "hide" goto statements among regular statements.
Quoted 6 lines
Quoted 1 line
IMO goto warrants a big flashing "notice me" sign.
Hmmm, perhaps.
So, it seems that we can reach an agreement. Any other comments or suggestions?
One more -- I absolutely agree with Jeff that goto should stand out as best as possible but I think that's actually more so when they're indented 2 columns.
Have been working on a legacy CD-ROM driver lately and puting "out:" labels at 2 spaces started out as a personal style preference of Pekka Enberg (I used to put them at 0) but has grown on me. It makes them clearly fall inside the function,
not being aligned at the same level as the next function header, which makes
for the "lowest effort visual scan" of all I feel. One is just too little for that, more than 2 is too much...
Here's the last version that was posted:
http://lkml.org/lkml/2007/6/4/50
It gets a little different visually when labels are mostly longer than a simple "out" or "again", or "error" or something like that but if someone's going to try to pin down the label style, I'd like the freedom to have two spaces in front of them...
Rene.
P.S: Your message had a Mail-Followup-To set which dropped yourself and turned the others from CCs into TOs. If you can help it, please no header tricks.
Quoted 31 lines · Rene Herman
Quoted 9 lines · WANG Cong
Quoted 7 lines · Jeff Garzik
A matter of opinion :) I tend to think goto is special enough to warrant column 1 unconditionally. It is special, so it draws additional attention over and above case labels.
I and others have been tripped up when programmers "hide" goto statements among regular statements.
Quoted 6 lines
Quoted 1 line
IMO goto warrants a big flashing "notice me" sign.
Hmmm, perhaps.
So, it seems that we can reach an agreement. Any other comments or suggestions?
One more -- I absolutely agree with Jeff that goto should stand out as best as possible but I think that's actually more so when they're indented 2 columns.
Have been working on a legacy CD-ROM driver lately and puting "out:" labels at 2 spaces started out as a personal style preference of Pekka Enberg (I used to put them at 0) but has grown on me. It makes them clearly fall inside the function, not being aligned at the same level as the next function header, which makes for the "lowest effort visual scan" of all I feel. One is just too little for that, more than 2 is too much...
Here's the last version that was posted:
Quoted 8 lines
http://lkml.org/lkml/2007/6/4/50
It gets a little different visually when labels are mostly longer than a simple "out" or "again", or "error" or something like that but if someone's going to try to pin down the label style, I'd like the freedom to have two spaces in front of them...
Rene.
I see. Thank you for your advice. Freedom is a good thing. ;)
Quoted 4 lines
P.S: Your message had a Mail-Followup-To set which dropped yourself and turned the others from CCs into TOs. If you can help it, please no header tricks.
Thanks! You are very kind. I will try to fix it!
Regards!