Re: [Qemu-trivial] [Qemu-devel] [PATCH v3 for-2.2 0/8] don't use Yoda conditions

[Markus Armbruster]

"Michael S. Tsirkin" <address@hidden> writes:

> 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.
>> Therefore:
>
> 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.

Actually, the relation is clear to me.  The patch cleans up style, by
converting Yoda conditionals into the locally appropriate form.

"Locally appropriate" because the optimal choice between "x == NULL" and
"!x" depends on which form the context uses.

Gonglei made those choices.  It's only a small improvement, and I
wouldn't demand it, but since we already have it, let's not throw it
away.

> 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.

Nope, because there's no consensus.  See thread leading to
https://lists.nongnu.org/archive/html/qemu-devel/2014-08/msg00033.html

> 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.

Hope you have better luck than me, because when I try to route tree-wide
cleanups via -trivial, I usually don't have any.