[PATCH] CodingStyle: proscribe do-while without braces.

[PATCH] CodingStyle: proscribe do-while without braces.

[Josh Triplett]

Sparse warns about do-while loops without braces; Linus's rationale from the
Sparse Git changelog:
> Add warning message for naked do-while
>
> Does it necessarily make sense? Dunno, but it does tend to be bad
> practice, or at least result in code that can be hard to mentally parse.
>
> Maybe that mental parsing is just me.	 Or maybe it should be warned
> about. You decide.

Signed-off-by: Josh Triplett <josh@kernel.org>
---
 Documentation/CodingStyle |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
index 7f1730f..f12e4b8 100644
--- a/Documentation/CodingStyle
+++ b/Documentation/CodingStyle
@@ -175,6 +175,13 @@ if (condition) {
 	otherwise();
 }
 
+This also does not apply to a do-while loop; always use braces with a do-while,
+even if it contains a single statement:
+
+do {
+	this();
+} while(condition);
+
 		3.1:  Spaces
 
 Linux kernel style for use of spaces depends (mostly) on
-- 
1.5.2.1

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Andrew Morton]

On Thu, 26 Jul 2007 14:37:02 -0700 Josh Triplett <josht@linux.vnet.ibm.com> wrote:

> Sparse warns about do-while loops without braces; Linus's rationale from the
> Sparse Git changelog:
> > Add warning message for naked do-while
> >
> > Does it necessarily make sense? Dunno, but it does tend to be bad
> > practice, or at least result in code that can be hard to mentally parse.
> >
> > Maybe that mental parsing is just me.	 Or maybe it should be warned
> > about. You decide.
> 
> Signed-off-by: Josh Triplett <josh@kernel.org>
> ---
>  Documentation/CodingStyle |    7 +++++++
>  1 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> index 7f1730f..f12e4b8 100644
> --- a/Documentation/CodingStyle
> +++ b/Documentation/CodingStyle
> @@ -175,6 +175,13 @@ if (condition) {
>  	otherwise();
>  }
>  
> +This also does not apply to a do-while loop; always use braces with a do-while,
> +even if it contains a single statement:
> +
> +do {
> +	this();
> +} while(condition);
> +

err, your example has broken coding style.

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Josh Triplett]

On Thu, 2007-07-26 at 14:44 -0700, Andrew Morton wrote:
> On Thu, 26 Jul 2007 14:37:02 -0700 Josh Triplett <josht@linux.vnet.ibm.com> wrote:
> 
> > Sparse warns about do-while loops without braces; Linus's rationale from the
> > Sparse Git changelog:
> > > Add warning message for naked do-while
> > >
> > > Does it necessarily make sense? Dunno, but it does tend to be bad
> > > practice, or at least result in code that can be hard to mentally parse.
> > >
> > > Maybe that mental parsing is just me.	 Or maybe it should be warned
> > > about. You decide.
> > 
> > Signed-off-by: Josh Triplett <josh@kernel.org>
> > ---
> >  Documentation/CodingStyle |    7 +++++++
> >  1 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/Documentation/CodingStyle b/Documentation/CodingStyle
> > index 7f1730f..f12e4b8 100644
> > --- a/Documentation/CodingStyle
> > +++ b/Documentation/CodingStyle
> > @@ -175,6 +175,13 @@ if (condition) {
> >  	otherwise();
> >  }
> >  
> > +This also does not apply to a do-while loop; always use braces with a do-while,
> > +even if it contains a single statement:
> > +
> > +do {
> > +	this();
> > +} while(condition);
> > +
> 
> err, your example has broken coding style.

Oops; thanks for catching that. :)

- Josh Triplett

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Krzysztof Halasa]

Josh Triplett <josht@linux.vnet.ibm.com> writes:

> +This also does not apply to a do-while loop; always use braces with a do-while,
> +even if it contains a single statement:

I can't see anything wrong with

do
	abc;
while (xyz);


and even if I could, "always use" seems way too strong in this case.
-- 
Krzysztof Halasa

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Andrew Morton]

On Fri, 27 Jul 2007 02:18:34 +0200 Krzysztof Halasa <khc@pm.waw.pl> wrote:

> Josh Triplett <josht@linux.vnet.ibm.com> writes:
> 
> > +This also does not apply to a do-while loop; always use braces with a do-while,
> > +even if it contains a single statement:
> 
> I can't see anything wrong with
> 
> do
> 	abc;
> while (xyz);
> 

Me either, but whatever.

> and even if I could, "always use" seems way too strong in this case.

it's better that we all do things the same way.  What that way _is_ is
actually less important, unless it's something stupid, of course.

In this case, most of the code I've seen uses the braces (I think).

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Stefan Richter]

Krzysztof Halasa wrote:
> and even if I could, "always use" seems way too strong in this case.

Besides, how about reorienting the CodingStyle text to the essential?

(But leave the humorous language in --- the few parts that are inherited
from old versions of that file, now more and more being buried in dry
nitpicking.)
-- 
Stefan Richter
-=====-=-=== -=== ==-==
http://arcgraph.de/sr/

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Andrew Morton]

On Fri, 27 Jul 2007 02:42:58 +0200 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Krzysztof Halasa wrote:
> > and even if I could, "always use" seems way too strong in this case.
> 
> Besides, how about reorienting the CodingStyle text to the essential?
> 
> (But leave the humorous language in --- the few parts that are inherited
> from old versions of that file, now more and more being buried in dry
> nitpicking.)

If sparse has gone and added a warning for this then it is more than
nitpicking.  Reducing the amount of noise coming out of sparse is useful.

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Randy Dunlap]

On Fri, 27 Jul 2007 02:42:58 +0200 Stefan Richter wrote:

> Krzysztof Halasa wrote:
> > and even if I could, "always use" seems way too strong in this case.
> 
> Besides, how about reorienting the CodingStyle text to the essential?

I don't mind that.  Send patch(es).

> (But leave the humorous language in --- the few parts that are inherited
> from old versions of that file, now more and more being buried in dry
> nitpicking.)

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Stefan Richter]

Andrew Morton wrote:
> On Fri, 27 Jul 2007 02:42:58 +0200 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
>> Besides, how about reorienting the CodingStyle text to the essential?
...
> If sparse has gone and added a warning for this then it is more than
> nitpicking.  Reducing the amount of noise coming out of sparse is useful.

Sure, that'd be useful.  But then, SubmitChecklist already tells us to
check with sparse before submission.
-- 
Stefan Richter
-=====-=-=== -=== ==-==
http://arcgraph.de/sr/

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Andrew Morton]

On Fri, 27 Jul 2007 09:41:01 +0200 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:

> Andrew Morton wrote:
> > On Fri, 27 Jul 2007 02:42:58 +0200 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
> >> Besides, how about reorienting the CodingStyle text to the essential?
> ...
> > If sparse has gone and added a warning for this then it is more than
> > nitpicking.  Reducing the amount of noise coming out of sparse is useful.
> 
> Sure, that'd be useful.  But then, SubmitChecklist already tells us to
> check with sparse before submission.

rofl.  First we have to talk everyone into checking their stuff with gcc.

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Krzysztof Halasa]

Andrew Morton <akpm@linux-foundation.org> writes:

> it's better that we all do things the same way.  What that way _is_ is
> actually less important, unless it's something stupid, of course.

It's certainly true WRT things like indentation but IMHO it shouldn't
go that far, and if it goes, it should be non-braced version.

If we prefer non-braced versions of "if" and "while", it would be
a bit strange to require braces with "do while", wouldn't it?

Sparse warnings... I think it shouldn't complain either, unless
called with extra parameter.


Perhaps some pointer to a bad-looking example so I can see for myself?
-- 
Krzysztof Halasa

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Josh Triplett]

On Fri, 2007-07-27 at 18:00 +0200, Krzysztof Halasa wrote:
> Andrew Morton <akpm@linux-foundation.org> writes:
> 
> > it's better that we all do things the same way.  What that way _is_ is
> > actually less important, unless it's something stupid, of course.
> 
> It's certainly true WRT things like indentation but IMHO it shouldn't
> go that far, and if it goes, it should be non-braced version.
> 
> If we prefer non-braced versions of "if" and "while", it would be
> a bit strange to require braces with "do while", wouldn't it?

Perhaps, but
     1. Existing kernel style uses braces with single-statement
        do-while.
     2. Linus prefers (or at least preferred in October of 2006) braces
        with single-statement do-while.

> Sparse warnings... I think it shouldn't complain either, unless
> called with extra parameter.

Good point; Sparse shouldn't warn about this by default.  I've turned
that off in latest Sparse from Git, so you need to give -Wdo-while or
-Wall to get warnings about that.  However, the kernel gives -Wall, so
you'll still see the warnings there.

- Josh Triplett

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Stefan Richter]

Andrew Morton wrote:
> On Fri, 27 Jul 2007 09:41:01 +0200 Stefan Richter <stefanr@s5r6.in-berlin.de> wrote:
...
>> SubmitChecklist already tells us to check with sparse before submission.
> 
> rofl.  First we have to talk everyone into checking their stuff with gcc.

Ah, I forgot --- Real Submitters don't compile their code before
submission.  But wait, they also don't track CodingStyle.

Seriously though:  Precise rules are a priori welcome.  But the more
rules there are, the less of them we (authors, reviewers) can memorize.
If we go for having many rules, then we need to rely more on checkpatch
and sparse.  When growing into a catch-all rule book, the utility of
CodingStyle will shift from an author's guide to a specification for
code checking tools.
-- 
Stefan Richter
-=====-=-=== -=== ==-==
http://arcgraph.de/sr/

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Krzysztof Halasa]

Josh Triplett <josht@linux.vnet.ibm.com> writes:

> Perhaps, but
>      1. Existing kernel style uses braces with single-statement
>         do-while.
>      2. Linus prefers (or at least preferred in October of 2006) braces
>         with single-statement do-while.

Fine with me, but it should IMHO be maintainer's call.

Things which a) are clearly better, and/or b) would require massive
reformatting when new maintainer steps in, should be documented
in CodingStyle and perhaps checked for by scripts and sparse.

Other things... sometimes there are multiple ways to do something
well, and humans aren't, and shouldn't be that deterministic.
-- 
Krzysztof Halasa

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Andrew Morton]

On Fri, 27 Jul 2007 10:05:16 -0700
Josh Triplett <josht@linux.vnet.ibm.com> wrote:

> > Sparse warnings... I think it shouldn't complain either, unless
> > called with extra parameter.
> 
> Good point; Sparse shouldn't warn about this by default.  I've turned
> that off in latest Sparse from Git, so you need to give -Wdo-while or
> -Wall to get warnings about that.  However, the kernel gives -Wall, so
> you'll still see the warnings there.

I believe reeducating sparse was the best fix here.  I think I'll now
have a quiet accident with codingstyle-proscribe-do-while-without-braces.patch

Re: [PATCH] CodingStyle: proscribe do-while without braces.

[Josh Triplett]

On Fri, 2007-07-27 at 11:27 -0700, Andrew Morton wrote:
> On Fri, 27 Jul 2007 10:05:16 -0700
> Josh Triplett <josht@linux.vnet.ibm.com> wrote:
> 
> > > Sparse warnings... I think it shouldn't complain either, unless
> > > called with extra parameter.
> > 
> > Good point; Sparse shouldn't warn about this by default.  I've turned
> > that off in latest Sparse from Git, so you need to give -Wdo-while or
> > -Wall to get warnings about that.  However, the kernel gives -Wall, so
> > you'll still see the warnings there.
> 
> I believe reeducating sparse was the best fix here.  I think I'll now
> have a quiet accident with codingstyle-proscribe-do-while-without-braces.patch

I didn't just submit the patch because Sparse warns about it.  I
submitted the patch because of the coding style preference that led to
the Sparse warning.

Also, if this really *shouldn't* form part of the kernel style, then the
kernel should use -Wno-do-while with -Wall, or should stop using -Wall.

That said, I don't care all that much about the patch, so I won't push
further for it one way or another.

- Josh Triplett