Re: [Qemu-devel] [PATCH] Drop braces around single statement rule

[Kevin Wolf]

Am 02.08.2010 17:20, schrieb Anthony Liguori:
> On 07/31/2010 06:49 PM, Aurelien Jarno wrote:
>> On Sat, Jul 31, 2010 at 08:23:55PM +0000, Blue Swirl wrote:
>>    
>>> On Sat, Jul 31, 2010 at 4:23 PM, malc<address@hidden>  wrote:
>>>      
>>>> History has shown that this particular rule is unenforcable.
>>>>
>>>> Signed-off-by: malc<address@hidden>
>>>> ---
>>>>   CODING_STYLE |   11 ++++++-----
>>>>   1 files changed, 6 insertions(+), 5 deletions(-)
>>>>        
>>> Not again:
>>> http://lists.nongnu.org/archive/html/qemu-devel/2009-12/msg00484.html
>>>
>>> There are plenty of ways to make the rule enforceable, for example we
>>> could agree to start to revert commits which introduce new
>>> CODING_STYLE violations.
>>>
>>>      
>> It seems to be possible to add a pre-applypatch script to the git hook
>> directory, that will verify the commit and reject it if it doesn't
>> comply with the coding rules. Of course it's possible to commit a patch
>> anyway by using --no-verify.
>>    
> 
> There are certain aspects of CODING_STYLE that I think are pretty 
> important.  For instance, space vs. tabs can really screw things up for 
> people that have non-standard tabs.  This is something that enforcing at 
> patch submission time seems to be really important.
> 
> Type naming seems important too because it's often not isolated.  IOW, a 
> poor choice in one file can end up polluting other files quickly that 
> require interacting.  The result is a mess of naming styles.
> 
> But something like braces around an if doesn't seem like it creates a 
> big problem.  Most C programmers are used to seeing braces in some 
> statements and not other.  Therefore, it's hard to argue that the code 
> gets really unreadable if this isn't strictly enforced.

I won't argue that missing braces impact readability of the code, they
probably don't. However, as was pointed out in earlier discussion there
still remain two important points:

1. While it doesn't make a difference for the code itself, readability
of patches suffers when braces have to be added/removed when a second
line is inserted or disappears.

2. I've messed up more than once with adding some debug code (even worse
when it happens with real code):

if (foo)
     fprintf(stderr, "debug msg\n");
     bar(); /* oops, not conditional any more */

This is why I tend to disagree with removing the rule, and suggest to
rather implement some automatic checks like Aurelien suggested (if we
need to change anything at all). I usually don't ask for a respin just
for braces if the patch is good otherwise, but if you think we should
just reject such patches without exceptions, I can change that.

> So really, I think the problem is that we're enforcing the words of
> CODING_STYLE instead of the intent.  The intent of CODING_STYLE is to 
> improve the readability of the code.  I think it requires a certain 
> amount of taste to be applied.
> 
> Rejecting a big patch because braces aren't used in single line if 
> statements seems to be an unnecessary barrier to me.

Taking such patches anyway is basically what we're doing today, right?
And what malc is complaining about.

Kevin