Re: [Qemu-devel] [PATCH 07/10] pseries: Clean up error handling in spapr_rtas_register()

[Markus Armbruster]

Alexey Kardashevskiy <address@hidden> writes:

> On 01/20/2016 06:18 PM, Markus Armbruster wrote:
>> Eric Blake <address@hidden> writes:
>>
>>> On 01/19/2016 05:21 PM, Alexey Kardashevskiy wrote:
>>>
>>>>> You could drop the redundant () while touching this, as in:
>>>>
>>>>
>>>> Seriously? Why? I personally find it really annoying (but I stay silent)
>>>> when people omit braces in cases like this.
>>>>
>>>>
>>>>> assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
>>>
>>> Because it's the prevailing style. I estimate that less than 10% of qemu
>>> over-parenthesizes, mostly because && and || are well-known C operator
>>> precedence:
>>>
>>> $ git grep ' && ' | wc
>>>     6462   57034  482477
>>> $ git grep ') && (' | wc
>>>      578    6151   48655
>>>
>>> Of course, that's a rough estimate, as it has false positives on 'if
>>> (foo() && (b || c))', and false negatives on conditionals where there is
>>> a unary rather than binary operator on either side of &&; but I'm sure
>>> you could write a Coccinelle script if you wanted more accurate counting.
>>>
>>> But you are equally right that as long as HACKING doesn't document it,
>>> and checkpatch.pl doesn't flag it, then you can over-parenthesize binary
>>> arguments to the short-circuiting operators to your aesthetic tastes.
>>
>> HACKING doesn't document everything.  Trying to document everything
>> would drown the interesting parts in a sea of platitudes, and still
>> leave innumerable loopholes.
>>
>> checkpatch.pl doesn't flag everything.  It checks for *common* unwanted
>> patterns.
>>
>> When HACKING and checkpatch.pl are silent, make your change blend in
>> with the existing code.  Since the existing code overwhelmingly eschews
>> this kind of superfluous parenthesis, the general rule is to knock them
>> off unless *local* code overwhelmingly uses them.
>
>
> In order to educate myself - where/when was this wonderful rule
> established? What are the other rules then?

The rule to make your new code blend in with the surrounding existing
code is common sense, and as such predates HACKING, or even QEMU.

If everybody did his own thing unless told otherwise in writing, we'd
end up with an incoherent mess[*].

I figure few people agree with every aspect of the prevailing QEMU
coding style.  I certainly don't.  But the development community largely
agrees that a reasonable level of stylistic consistency is wanted, and
worth some adjustment of habits and sacrifice of personal preferences.

Making your code blend in with the existing code requires a bit of care
and taste.  It doesn't require memorizing a long list of arbitrary
rules.  Give us your honest best effort, and respond constructively to
review comments, and you'll be fine.

[...]