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!
Signed-off-by: WANG Cong <[email protected]>
---
CodingStyle | 6 ++++++
1 file changed, 6 insertions(+)
--- linux-2.6.22-rc3/Documentation/CodingStyle.orig 2007-06-03 21:28:19.000000000 +0800
+++ linux-2.6.22-rc3/Documentation/CodingStyle 2007-06-03 21:51:21.000000000 +0800
@@ -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.
On Sun, Jun 03, 2007 at 10:24:50PM +0800, WANG Cong wrote:
> +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.
On Sun, 3 Jun 2007 15:29:39 +0100 Al Viro wrote:
> On Sun, Jun 03, 2007 at 10:24:50PM +0800, WANG Cong wrote:
>
> > +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 ***
Randy Dunlap wrote:
> On Sun, 3 Jun 2007 15:29:39 +0100 Al Viro wrote:
>
>> On Sun, Jun 03, 2007 at 10:24:50PM +0800, WANG Cong wrote:
>>
>>> +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.
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
On Sun, Jun 03, 2007 at 11:35:43AM -0400, Jeff Garzik wrote:
>Randy Dunlap wrote:
>>On Sun, 3 Jun 2007 15:29:39 +0100 Al Viro wrote:
>>
>>>On Sun, Jun 03, 2007 at 10:24:50PM +0800, WANG Cong wrote:
>>>
>>>>+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.
>
>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
WANG Cong wrote:
> 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
On Sun, Jun 03, 2007 at 11:59:26PM -0700, Jeremy Fitzhardinge wrote:
>WANG Cong wrote:
>> 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. ;)
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(+)
--- linux-2.6.22-rc3/Documentation/CodingStyle.orig 2007-06-04 15:53:58.000000000 +0800
+++ linux-2.6.22-rc3/Documentation/CodingStyle 2007-06-04 16:04:31.000000000 +0800
@@ -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.
Jeff Garzik wrote:
>
> 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
Jeremy Fitzhardinge wrote:
> WANG Cong wrote:
>> 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
On Jun 4 2007 10:27, H. Peter Anvin wrote:
>Jeff Garzik wrote:
>>
>> 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
--
Jan Engelhardt wrote:
> On Jun 4 2007 10:27, H. Peter Anvin wrote:
>> Jeff Garzik wrote:
>>> 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).
>
So?
You still want them to be associated with the level the bailout happens at.
-hpa
On Mon, 04 Jun 2007 10:43:51 PDT, "H. Peter Anvin" said:
> Jan Engelhardt wrote:
> > 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.
[email protected] wrote:
>>
>> 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
On Mon, Jun 04, 2007 at 04:19:38PM +0800, WANG Cong wrote:
>
> 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(+)
>
> --- linux-2.6.22-rc3/Documentation/CodingStyle.orig 2007-06-04 15:53:58.000000000 +0800
> +++ linux-2.6.22-rc3/Documentation/CodingStyle 2007-06-04 16:04:31.000000000 +0800
> @@ -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
H. Peter Anvin wrote:
> 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();
}
J
Sam Ravnborg wrote:
> 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
Jeremy Fitzhardinge wrote:
> H. Peter Anvin wrote:
>> 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
On Mon, Jun 04, 2007 at 10:43:51AM -0700, H. Peter Anvin wrote:
> Jan Engelhardt wrote:
> > On Jun 4 2007 10:27, H. Peter Anvin wrote:
> >> Jeff Garzik wrote:
> >>> 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).
> >
>
> 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
On Mon, Jun 04, 2007 at 12:02:28PM -0700, Jeremy Fitzhardinge wrote:
>Sam Ravnborg wrote:
>> 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.
On Mon, Jun 04, 2007 at 01:57:51PM -0400, Jeff Garzik wrote:
>On Mon, Jun 04, 2007 at 10:43:51AM -0700, H. Peter Anvin wrote:
>> Jan Engelhardt wrote:
>> > On Jun 4 2007 10:27, H. Peter Anvin wrote:
>> >> Jeff Garzik wrote:
>> >>> 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).
>> >
>>
>> 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
Hmmm, perhaps.
So, it seems that we can reach an agreement. Any other comments or suggestions?
Or can someone ack/merge this patch?
Thanks!
On Tue, Jun 05, 2007 at 10:10:27AM +0800, WANG Cong wrote:
> On Mon, Jun 04, 2007 at 01:57:51PM -0400, Jeff Garzik wrote:
> >On Mon, Jun 04, 2007 at 10:43:51AM -0700, H. Peter Anvin wrote:
> >> Jan Engelhardt wrote:
> >> > On Jun 4 2007 10:27, H. Peter Anvin wrote:
> >> >> Jeff Garzik wrote:
> >> >>> 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).
> >> >
> >>
> >> 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
>
> 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
Jeff Garzik wrote:
>>
>> 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.
>
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
On Mon, Jun 04, 2007 at 07:31:27PM -0700, H. Peter Anvin wrote:
>Jeff Garzik wrote:
>>>
>>> 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.
>>
>
>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
On 06/05/2007 04:10 AM, WANG Cong wrote:
> On Mon, Jun 04, 2007 at 01:57:51PM -0400, Jeff Garzik wrote:
>> 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.
> 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.
On Tue, Jun 05, 2007 at 11:16:13AM +0200, Rene Herman wrote:
>On 06/05/2007 04:10 AM, WANG Cong wrote:
>
>>On Mon, Jun 04, 2007 at 01:57:51PM -0400, Jeff Garzik wrote:
>
>>>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.
>
>>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.
I see. Thank you for your advice. Freedom is a good thing. ;)
>
>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!
On Tue, Jun 05, 2007 at 09:37:36AM +0800, WANG Cong wrote:
> 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...