Re: [Qemu-devel] [PATCH v2 1/8] CODING_STYLE: Section about conditional statement

[Gonglei (Arei)]

Hi,

> Subject: Re: [PATCH v2 1/8] CODING_STYLE: Section about conditional
> statement
> 
> On 07/31/2014 08:32 PM, address@hidden wrote:
> > From: Gonglei <address@hidden>
> >
> > Yoda conidtions lack of readability, and QEMU have a
> 
> s/conidtions/conditions/
> s/of //
> s/have/has/
> 
OK.

> > strict compiler configuration for checking a common
> > mistake like "if (dev = NULL)". Make it a written rule.
> >
> > Signed-off-by: Gonglei <address@hidden>
> > ---
> >  CODING_STYLE | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/CODING_STYLE b/CODING_STYLE
> > index 4280945..11a79d4 100644
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -91,3 +91,22 @@ Mixed declarations (interleaving statements and
> declarations within blocks)
> >  are not allowed; declarations should be at the beginning of blocks.  In
> other
> >  words, the code should not generate warnings if using GCC's
> >  -Wdeclaration-after-statement option.
> > +
> > +6. Conditional statement
> 
> s/statement/statements/
> 
OK.

> > +
> > +Please don't use Yoda conditions because of lack of readability. 
> > Furthermore,
> > +it is not the QEMU idiomatic coding style. Example:
> > +
> > +Usually a conditional statement in QEMU would be written as:
> > +if (a == 0) {
> > +    /* Reads like: "If a is equal to 0..." */
> > +    do_something();
> > +}
> > +
> > +Yoda conditions describe the same expression, but reversed:
> > +if (0 == a) {
> > +    /* Reads like: "If 0 equals to a" */
> > +    do_something();
> > +}
> > +
> > +The constant is listed first, then the variable being compared to.
> >
> 
> This spends more lines documenting the bad style than the good, and
> doesn't quite flow with the rest of the document.  At the risk of
> sounding like a complete rewrite, how about:
> 
> =====
> When comparing a variable for (in)equality with a constant, list the
> constant on the right, as in:
> 
> if (a == 0) {
>     do_something();
> }
> 
> Rationale: Yoda conditionals (as in 'if (0 == a)') are awkward to read.
> Besides, good compilers already warn users when == is mis-typed as =,
> even when the constant is on the right.
> =====
> 
Good description.

> and maybe some other ideas also worth adding:
> 
> =====
> Avoid redundant comparisons: (bool_expr == true) is better written as
> (bool_expr), and (ptr == NULL) is shorter as (!ptr).  Use of !!value is
> a convenient shorthand for converting a value into a boolean.
> =====
> 
Agreed. Thanks!

Best regards,
-Gonglei