[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 for-2.2 0/8] don't use Yoda co
Michael S. Tsirkin
Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 for-2.2 0/8] don't use Yoda conditions
Wed, 6 Aug 2014 08:57:42 +0200
On Wed, Aug 06, 2014 at 08:05:46AM +0200, Markus Armbruster wrote:
> "Gonglei (Arei)" <address@hidden> writes:
> > Hi,
> >> >
> >> > $WHATEVER: don't use 'Yoda conditions'
> >> >
> >> > 'Yoda conditions' are not part of idiomatic QEMU coding
> >> > style, so rewrite them in the more usual order.
> >> OK but why stop at these files? How about this
> >> instead?
> > I just search c files by using key words like "NULL ==" etc.
> > I don't think we should change conditional statements like ">" and ">=".
> Eric pointed out it's actually incorrect for NaNs.
> If you want to touch inequalities, separate patch(es) please, because
> they need more thorough review, both for correctness and for style.
> > BTW, just using like "value == NULL" instead of "NULL == value" in all files
> > is not a good idea, which we have discussed in my patch serials v2. So, I
> > posted
> > v3, add change log " imitate nearby code about using '!value' or
> > value == NULL' at
> > every patch " .
> Re "not a good idea": I think rewriting "NULL == value" to "value ==
> NULL" *is* a good idea, but rewriting it to "!value" where that blends
> in with surrounding code is a *better* idea.
> Gonglei's patches do that, Michael's don't, but are more complete.
Yes but it's unrelated to Yoda: we have x != NULL without Yoda
in a lot of places. So this seems, to me, an unrelated issue.
If people feel this == NULL -> !x is desired, it's better to do it all at
once IMHO, and do x != NULL -> x at the same time.
Easy to run another script to do it on top.
> > So, maybe you can post patches for those files I have missed in the
> > serials,
> > but not simply instead all by semantic script IMO, thanks!
> Easy: apply Gonglei's patches before you run the script.
> You may have to split patches along subsystem boundaries to get them in.
> Bothersome, as it involves guessing boundaries. Not a request from me,
> just a warning of possible misfortune :)
It's going in through trivial tree, I don't think split-up is necessary.