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<av1...@comtv.ru> wrote:
>>>
>>>> History has shown that this particular rule is unenforcable.
>>>>
>>>> Signed-off-by: malc<av1...@comtv.ru>
>>>> ---
>>>> 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
