[Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Michael S. Tsirkin]

Most people seem to hate using {} around sngle-statement blocks.
And code isn't consistent either way. So let's change our standard
to something most people like, and eliminate the pain source.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

The idea is to see which parts of linux kernel style we can pick without
much transitional pain.  Let's start small, the following seems to be
almost unanimously hated.

diff --git a/CODING_STYLE b/CODING_STYLE
index a579cb1..2e3ecba 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -51,19 +51,19 @@ QEMU coding style.
 
 4. Block structure
 
-Every indented statement is braced; even if the block contains just one
-statement.  The opening brace is on the line that contains the control
-flow statement that introduces the new block; the closing brace is on the
-same line as the else keyword, or on a line by itself if there is no else
-keyword.  Example:
+If an indented block contains just one statement, it is not braced.  This
+matches the Linux coding style.  The opening brace of a block is on the line
+that contains the control flow statement that introduces the new block; the
+closing brace is on the same line as the else keyword, or on a line by itself
+if there is no else keyword.  Example:
 
-    if (a == 5) {
+    if (a == 5)
         printf("a was 5.\n");
-    } else if (a == 6) {
+    else if (a == 6) {
         printf("a was 6.\n");
-    } else {
+        printf("multiply by 7 to get the answer.\n");
+    } else
         printf("a was something else entirely.\n");
-    }
 
 An exception is the opening brace for a function; for reasons of tradition
 and clarity it comes on a line by itself:
@@ -75,4 +75,5 @@ and clarity it comes on a line by itself:
 
 Rationale: a consistent (except for functions...) bracing style reduces
 ambiguity and avoids needless churn when lines are added or removed.
+This matches the linux coding style.
 Furthermore, it is the QEMU coding style.

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Stefan Weil]

Michael S. Tsirkin schrieb:
> Most people seem to hate using {} around sngle-statement blocks.
> And code isn't consistent either way. So let's change our standard
> to something most people like, and eliminate the pain source.
>
>   

Inconsistency is not an argument for or against {}.
I don't like single statement blocks without {}
because of my personal experience with all kinds
of code standard variants.

Please don't change the qemu standard, I like it as it is.

Stefan Weil

> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>   

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Anthony Liguori]

Stefan Weil wrote:
> Michael S. Tsirkin schrieb:
>   
>> Most people seem to hate using {} around sngle-statement blocks.
>> And code isn't consistent either way. So let's change our standard
>> to something most people like, and eliminate the pain source.
>>
>>   
>>     
>
> Inconsistency is not an argument for or against {}.
> I don't like single statement blocks without {}
> because of my personal experience with all kinds
> of code standard variants.
>
> Please don't change the qemu standard, I like it as it is.
>   

I've never liked this but I would rather not see unnecessary churn so 
I'd prefer to keep the coding style as-is.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Markus Armbruster]

Anthony Liguori <anthony@codemonkey.ws> writes:

> Stefan Weil wrote:
>> Michael S. Tsirkin schrieb:
>>   
>>> Most people seem to hate using {} around sngle-statement blocks.
>>> And code isn't consistent either way. So let's change our standard
>>> to something most people like, and eliminate the pain source.
>>>
>>>       
>>
>> Inconsistency is not an argument for or against {}.
>> I don't like single statement blocks without {}
>> because of my personal experience with all kinds
>> of code standard variants.
>>
>> Please don't change the qemu standard, I like it as it is.
>>   
>
> I've never liked this but I would rather not see unnecessary churn so
> I'd prefer to keep the coding style as-is.

The existing code uses both brace styles.  We didn't convert it
wholesale when the "thou shalt use braces" rule was written into the
coding style, because we didn't want the churn.  We can handle it just
the same now.  Churn is not an argument then.

I dislike redundant braces, and support the change Michael proposed.

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Stuart Brady]

On Tue, Oct 06, 2009 at 09:01:15PM +0200, Michael S. Tsirkin wrote:
> Most people seem to hate using {} around sngle-statement blocks.
> And code isn't consistent either way. So let's change our standard
> to something most people like, and eliminate the pain source.

I can't see how this is really an improvement.  All it means is
that patches have to deal with adding extra '{'s and '}'s, which
is a pointless distraction when reading patches...

Regards,
-- 
Stuart Brady

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Jamie Lokier]

Michael S. Tsirkin wrote:
>  4. Block structure
>  
> -Every indented statement is braced; even if the block contains just one
> -statement.  The opening brace is on the line that contains the control
> -flow statement that introduces the new block; the closing brace is on the
> -same line as the else keyword, or on a line by itself if there is no else
> -keyword.  Example:
> +If an indented block contains just one statement, it is not braced.  This
> +matches the Linux coding style.  The opening brace of a block is on the line
> +that contains the control flow statement that introduces the new block; the
> +closing brace is on the same line as the else keyword, or on a line by itself
> +if there is no else keyword.  Example:
>  
> -    if (a == 5) {
> +    if (a == 5)
>          printf("a was 5.\n");
> -    } else if (a == 6) {
> +    else if (a == 6) {
>          printf("a was 6.\n");
> -    } else {
> +        printf("multiply by 7 to get the answer.\n");
> +    } else
>          printf("a was something else entirely.\n");
> -    }
>  
>  An exception is the opening brace for a function; for reasons of tradition
>  and clarity it comes on a line by itself:
> @@ -75,4 +75,5 @@ and clarity it comes on a line by itself:
>  
>  Rationale: a consistent (except for functions...) bracing style reduces
>  ambiguity and avoids needless churn when lines are added or removed.
> +This matches the linux coding style.

Actually the above does not match the Linux coding style, and it's ugly too.

This is the Linux style:

	Do not unnecessarily use braces where a single statement will do.

	if (condition)
		action();

	This does not apply if one branch of a conditional statement
	is a single statement. Use braces in both branches.

	if (condition) {
		do_this();
		do_that();
	} else {
		otherwise();
	}

-- Jamie

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Kevin Wolf]

Am 06.10.2009 21:01, schrieb Michael S. Tsirkin:
> Most people seem to hate using {} around sngle-statement blocks.
> And code isn't consistent either way. So let's change our standard
> to something most people like, and eliminate the pain source.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> ---
> 
> The idea is to see which parts of linux kernel style we can pick without
> much transitional pain.  Let's start small, the following seems to be
> almost unanimously hated.
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> index a579cb1..2e3ecba 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -51,19 +51,19 @@ QEMU coding style.
>  
>  4. Block structure
>  
> -Every indented statement is braced; even if the block contains just one
> -statement.  The opening brace is on the line that contains the control
> -flow statement that introduces the new block; the closing brace is on the
> -same line as the else keyword, or on a line by itself if there is no else
> -keyword.  Example:
> +If an indented block contains just one statement, it is not braced.  This
> +matches the Linux coding style.  The opening brace of a block is on the line
> +that contains the control flow statement that introduces the new block; the
> +closing brace is on the same line as the else keyword, or on a line by itself
> +if there is no else keyword.  Example:
>  
> -    if (a == 5) {
> +    if (a == 5)
>          printf("a was 5.\n");
> -    } else if (a == 6) {
> +    else if (a == 6) {
>          printf("a was 6.\n");
> -    } else {
> +        printf("multiply by 7 to get the answer.\n");
> +    } else
>          printf("a was something else entirely.\n");
> -    }

This is the best example of how having braces only sometimes makes
patches unreadable. All of the if/else lines are touched even though the
condition (and thus semantics) remains unchanged. Other than in patches
I really don't care much which way the code is written, but considering
that patches are not completely unimportant for us we might want to take
this into account.

But if we went for the change, please take Jamie's version and put
braces on all branches or on no branches, but don't mix.

>  
>  An exception is the opening brace for a function; for reasons of tradition
>  and clarity it comes on a line by itself:
> @@ -75,4 +75,5 @@ and clarity it comes on a line by itself:
>  
>  Rationale: a consistent (except for functions...) bracing style reduces
>  ambiguity and avoids needless churn when lines are added or removed.

Well, this sentence wouldn't apply anymore, so you should remove it.

> +This matches the linux coding style.
>  Furthermore, it is the QEMU coding style.

Kevin

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Gerd Hoffmann]

>> -    if (a == 5) {
>> +    if (a == 5)
>>           printf("a was 5.\n");
>> -    } else if (a == 6) {
>> +    else if (a == 6) {
>>           printf("a was 6.\n");
>> -    } else {
>> +        printf("multiply by 7 to get the answer.\n");
>> +    } else
>>           printf("a was something else entirely.\n");
>> -    }
>
> This is the best example of how having braces only sometimes makes
> patches unreadable. All of the if/else lines are touched even though the
> condition (and thus semantics) remains unchanged.

I somehow dislike the unneeded branches because it looks a bit 
irritating to my eyes.

They have advantages though.  Making patches more readable is one, as 
kevin and the nice example above clearly points out ;)

Another one is that you don't have to hop around in your editor adding 
and removing branches when throwing in a temporary debug printf.

So I'd tend to keep them.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] CODING_STYLE: {} as in linux kernel

[Stuart Brady]

On Wed, Oct 07, 2009 at 04:54:24PM +0200, Gerd Hoffmann wrote:

> I somehow dislike the unneeded branches because it looks a bit 
> irritating to my eyes.

I felt the same way at first, but I've become used to them, now.

> They have advantages though.  Making patches more readable is one, as 
> kevin and the nice example above clearly points out ;)

Strictly, the example should be this:

-    if (a == 5)
+    if (a == 5) {
         printf("a was 5.\n");
-    else if (a == 6)
+    } else if (a == 6) {
         printf("a was 6.\n");
+        printf("multiply by 7 to get the answer.\n");
-    else
+    } else {
         printf("a was something else entirely.\n");
+    }

versus:

     if (a == 5) {
         printf("a was 5.\n");
     } else if (a == 6) {
         printf("a was 6.\n");
+        printf("multiply by 7 to get the answer.\n");
     } else {
         printf("a was something else entirely.\n");
     }

I know which one I prefer. :-)

Cheers,
-- 
Stuart Brady