login
Header Space

 
 

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

Previous thread: [PATCH] ia64: fix a few section mismatch warnings by Sam Ravnborg on Thursday, July 26, 2007 - 5:01 pm. (10 messages)

Next thread: [PATCH][dccp] Fix memory leak and clean up style - dccp_feat_empty_confirm() by Jesper Juhl on Thursday, July 26, 2007 - 6:39 pm. (2 messages)
To: <linux-kernel@...>
Cc: Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 5:37 pm

Sparse warns about do-while loops without braces; Linus's rationale from the

Signed-off-by: Josh Triplett &lt;josh@kernel.org&gt;
---
 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


-
To: Josh Triplett <josht@...>
Cc: <linux-kernel@...>, Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 8:18 pm

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
-
To: Krzysztof Halasa <khc@...>
Cc: Josh Triplett <josht@...>, <linux-kernel@...>, Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 8:42 pm

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/
-
To: Stefan Richter <stefanr@...>
Cc: Krzysztof Halasa <khc@...>, Josh Triplett <josht@...>, <linux-kernel@...>, Andrew Morton <akpm@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 9:17 pm

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***
-
To: Stefan Richter <stefanr@...>
Cc: Krzysztof Halasa <khc@...>, Josh Triplett <josht@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 8:59 pm

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.
-
To: Andrew Morton <akpm@...>
Cc: Krzysztof Halasa <khc@...>, Josh Triplett <josht@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 3:41 am

Sure, that'd be useful.  But then, SubmitChecklist already tells us to
check with sparse before submission.
-- 
Stefan Richter
-=====-=-=== -=== ==-==
http://arcgraph.de/sr/
-
To: Stefan Richter <stefanr@...>
Cc: Krzysztof Halasa <khc@...>, Josh Triplett <josht@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 3:53 am

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

-
To: Andrew Morton <akpm@...>
Cc: Krzysztof Halasa <khc@...>, Josh Triplett <josht@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 1:13 pm

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/
-
To: Krzysztof Halasa <khc@...>
Cc: Josh Triplett <josht@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 8:35 pm

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).

-
To: Andrew Morton <akpm@...>
Cc: Josh Triplett <josht@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 12:00 pm

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
-
To: Krzysztof Halasa <khc@...>
Cc: Andrew Morton <akpm@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 1:05 pm

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

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


-
To: Josh Triplett <josht@...>
Cc: Krzysztof Halasa <khc@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 2:27 pm

On Fri, 27 Jul 2007 10:05:16 -0700

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
-
To: Andrew Morton <akpm@...>
Cc: Krzysztof Halasa <khc@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 2:52 pm

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


-
To: Josh Triplett <josht@...>
Cc: Andrew Morton <akpm@...>, <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Friday, July 27, 2007 - 1:44 pm

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
-
To: Josh Triplett <josht@...>
Cc: <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 5:44 pm

err, your example has broken coding style.
-
To: Andrew Morton <akpm@...>
Cc: <linux-kernel@...>, Linus Torvalds <torvalds@...>, <linux-sparse@...>
Date: Thursday, July 26, 2007 - 5:54 pm

Oops; thanks for catching that. :)

- Josh Triplett


-
Previous thread: [PATCH] ia64: fix a few section mismatch warnings by Sam Ravnborg on Thursday, July 26, 2007 - 5:01 pm. (10 messages)

Next thread: [PATCH][dccp] Fix memory leak and clean up style - dccp_feat_empty_confirm() by Jesper Juhl on Thursday, July 26, 2007 - 6:39 pm. (2 messages)
speck-geostationary