|
From: | Anthony Liguori |
Subject: | Re: [Qemu-devel] [PATCH] Drop braces around single statement rule |
Date: | Mon, 02 Aug 2010 10:48:27 -0500 |
User-agent: | Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.11) Gecko/20100713 Lightning/1.0b1 Thunderbird/3.0.6 |
On 08/02/2010 10:41 AM, Kevin Wolf wrote:
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.
I understand the argument but it's not something that I strongly agree with.
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 hard to do with editors that auto indent unless you're copying and pasting debugging. And yeah, I've made that mistake too doing the later :-)
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.
Yeah, I try to remember to enforce it but often forget or just don't notice. My eyes don't tend to catch missing braces like they would catch bad type naming because the former is really not uncommon.
I'm happy with the status quo but I won't object to a git commit hook that enforces style.
Regards, Anthony Liguori
[Prev in Thread] | Current Thread | [Next in Thread] |