From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=53682 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ofx9M-0007Jj-Oy for qemu-devel@nongnu.org; Mon, 02 Aug 2010 11:42:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Ofx9H-0003Ld-ND for qemu-devel@nongnu.org; Mon, 02 Aug 2010 11:41:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6511) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Ofx9H-0003LP-GF for qemu-devel@nongnu.org; Mon, 02 Aug 2010 11:41:55 -0400 Message-ID: <4C56E732.7060102@redhat.com> Date: Mon, 02 Aug 2010 17:41:38 +0200 From: Kevin Wolf MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH] Drop braces around single statement rule References: <1280593414-2232-1-git-send-email-av1474@comtv.ru> <20100731234956.GJ31640@hall.aurel32.net> <4C56E253.7010403@codemonkey.ws> In-Reply-To: <4C56E253.7010403@codemonkey.ws> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Anthony Liguori Cc: Blue Swirl , qemu-devel@nongnu.org, Aurelien Jarno 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 wrote: >>> >>>> History has shown that this particular rule is unenforcable. >>>> >>>> Signed-off-by: malc >>>> --- >>>> 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