| Message ID | 1452859244-9500-8-git-send-email-david@gibson.dropbear.id.au |
|---|---|
| State | New |
| Headers | show |
On 01/15/2016 05:00 AM, David Gibson wrote: > The errors detected in this function necessarily indicate bugs in the rest > of the qemu code, rather than an external or configuration problem. > > So, a simple assert() is more appropriate than any more complex error > reporting. > > Signed-off-by: David Gibson <david@gibson.dropbear.id.au> > --- > hw/ppc/spapr_rtas.c | 12 +++--------- > 1 file changed, 3 insertions(+), 9 deletions(-) > > diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c > index 34b12a3..0be52ae 100644 > --- a/hw/ppc/spapr_rtas.c > +++ b/hw/ppc/spapr_rtas.c > @@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr, > > void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn) > { > - if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) { > - fprintf(stderr, "RTAS invalid token 0x%x\n", token); > - exit(1); > - } > + assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX)); You could drop the redundant () while touching this, as in: assert(token >= RTAS_TOKEN_BASE && token < RTAS_TOKEN_MAX);
On 01/20/2016 09:58 AM, Eric Blake wrote: > On 01/15/2016 05:00 AM, David Gibson wrote: >> The errors detected in this function necessarily indicate bugs in the rest >> of the qemu code, rather than an external or configuration problem. >> >> So, a simple assert() is more appropriate than any more complex error >> reporting. >> >> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> >> --- >> hw/ppc/spapr_rtas.c | 12 +++--------- >> 1 file changed, 3 insertions(+), 9 deletions(-) >> >> diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c >> index 34b12a3..0be52ae 100644 >> --- a/hw/ppc/spapr_rtas.c >> +++ b/hw/ppc/spapr_rtas.c >> @@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr, >> >> void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn) >> { >> - if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) { >> - fprintf(stderr, "RTAS invalid token 0x%x\n", token); >> - exit(1); >> - } >> + assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX)); > > 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); >
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. And for other operators, like '&' and '|', I definitely recommend the parenthesis, particularly if you manage to trigger a gcc or clang warning (in spite of the precedence being unambiguous) if you omit the parenthesis.
On 01/20/2016 03:53 PM, Eric Blake wrote: > 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. C Operator Precedence is well-known and still confusing, I cannot get used to the fact that </>/==/etc have higher priority than &/&&/etc so not seeing braces in the cases like above makes me nervous. Yes, I am sort of retarded :( So, we can keep doing this over-parenthesizing, good, thanks :) > And for other operators, like '&' and '|', I definitely recommend the > parenthesis, particularly if you manage to trigger a gcc or clang > warning (in spite of the precedence being unambiguous) if you omit the > parenthesis. Goood.
Eric Blake <eblake@redhat.com> 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. Just because HACKING doesn't explicitly prohibit your personal preferences doesn't mean you get to do leave your stylistic mark on the code. Show some taste and make yourself invisible. [...]
On 01/20/2016 06:18 PM, Markus Armbruster wrote: > Eric Blake <eblake@redhat.com> 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? > Just because HACKING doesn't explicitly prohibit your personal > preferences doesn't mean you get to do leave your stylistic mark on the > code. Show some taste and make yourself invisible. Nice, now we are discussing taste :-/
Alexey Kardashevskiy <aik@ozlabs.ru> writes: > On 01/20/2016 06:18 PM, Markus Armbruster wrote: >> Eric Blake <eblake@redhat.com> 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. [...]
On 20.01.2016 06:53, Alexey Kardashevskiy wrote: > On 01/20/2016 03:53 PM, Eric Blake wrote: >> 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. > > C Operator Precedence is well-known and still confusing, I cannot get > used to the fact that </>/==/etc have higher priority than &/&&/etc so > not seeing braces in the cases like above makes me nervous. Yes, I am > sort of retarded :( > > So, we can keep doing this over-parenthesizing, good, thanks :) For me, it's the other way round: If I notice too many parentheses while reading source code, I have to start thinking - because I then assume that there is something special with the statement so that the parentheses are needed. If I then discover that it was just unnecessary waste of time, I start complaining... So please try to get rid of your parenthesitis, or you've got to live with my complaints ;-) Thomas
diff --git a/hw/ppc/spapr_rtas.c b/hw/ppc/spapr_rtas.c index 34b12a3..0be52ae 100644 --- a/hw/ppc/spapr_rtas.c +++ b/hw/ppc/spapr_rtas.c @@ -648,17 +648,11 @@ target_ulong spapr_rtas_call(PowerPCCPU *cpu, sPAPRMachineState *spapr, void spapr_rtas_register(int token, const char *name, spapr_rtas_fn fn) { - if (!((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX))) { - fprintf(stderr, "RTAS invalid token 0x%x\n", token); - exit(1); - } + assert((token >= RTAS_TOKEN_BASE) && (token < RTAS_TOKEN_MAX)); token -= RTAS_TOKEN_BASE; - if (rtas_table[token].name) { - fprintf(stderr, "RTAS call \"%s\" is registered already as 0x%x\n", - rtas_table[token].name, token); - exit(1); - } + + assert(!rtas_table[token].name); rtas_table[token].name = name; rtas_table[token].fn = fn;
The errors detected in this function necessarily indicate bugs in the rest of the qemu code, rather than an external or configuration problem. So, a simple assert() is more appropriate than any more complex error reporting. Signed-off-by: David Gibson <david@gibson.dropbear.id.au> --- hw/ppc/spapr_rtas.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)