| Message ID | 1406860365-5516-3-git-send-email-arei.gonglei@huawei.com |
|---|---|
| State | New |
| Headers | show |
On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote: > From: Gonglei <arei.gonglei@huawei.com> > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > --- > hw/usb/dev-audio.c | 2 +- > hw/usb/dev-mtp.c | 4 ++-- > hw/usb/hcd-ehci.c | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c > index bfebfe9..988f6cc 100644 > --- a/hw/usb/dev-audio.c > +++ b/hw/usb/dev-audio.c > @@ -371,7 +371,7 @@ static void output_callback(void *opaque, int avail) > return; > } > data = streambuf_get(&s->out.buf); > - if (NULL == data) { > + if (data == NULL) { Wouldn't it be even more idiomatic as: if (!data) { Probably applies throughout your series.
Hi, > Subject: Re: [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing > style > > On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote: > > From: Gonglei <arei.gonglei@huawei.com> > > > > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > > --- > > hw/usb/dev-audio.c | 2 +- > > hw/usb/dev-mtp.c | 4 ++-- > > hw/usb/hcd-ehci.c | 2 +- > > 3 files changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c > > index bfebfe9..988f6cc 100644 > > --- a/hw/usb/dev-audio.c > > +++ b/hw/usb/dev-audio.c > > @@ -371,7 +371,7 @@ static void output_callback(void *opaque, int avail) > > return; > > } > > data = streambuf_get(&s->out.buf); > > - if (NULL == data) { > > + if (data == NULL) { > > Wouldn't it be even more idiomatic as: > > if (!data) { > > Probably applies throughout your series. > OK, will do. Thanks! > -- > Eric Blake eblake redhat com +1-919-301-3266 > Libvirt virtualization library http://libvirt.org Best regards, -Gonglei
Am 01.08.2014 05:32, schrieb Gonglei (Arei): > Hi, > >> Subject: Re: [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing >> style >> >> On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote: >>> From: Gonglei <arei.gonglei@huawei.com> >>> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>> --- >>> hw/usb/dev-audio.c | 2 +- >>> hw/usb/dev-mtp.c | 4 ++-- >>> hw/usb/hcd-ehci.c | 2 +- >>> 3 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c >>> index bfebfe9..988f6cc 100644 >>> --- a/hw/usb/dev-audio.c >>> +++ b/hw/usb/dev-audio.c >>> @@ -371,7 +371,7 @@ static void output_callback(void *opaque, int avail) >>> return; >>> } >>> data = streambuf_get(&s->out.buf); >>> - if (NULL == data) { >>> + if (data == NULL) { >> >> Wouldn't it be even more idiomatic as: >> >> if (!data) { >> >> Probably applies throughout your series. >> > OK, will do. Thanks! Not so quick! You are free to use that in your patches, but please don't change all code that way without the author's consent. Just like "equals null" is a natural English way of reading, compared to "null equals something", "not null" reads like a boolean expression to me, and even worse while all valid C, "not strcmp" leads to mind-boggling inverted logic... Regards, Andreas
Hi, > Subject: Re: [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing > style > > Am 01.08.2014 05:32, schrieb Gonglei (Arei): > > Hi, > > > >> Subject: Re: [PATCH v2 2/8] usb: a trivial code change for more idiomatic > writing > >> style > >> > >> On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote: > >>> From: Gonglei <arei.gonglei@huawei.com> > >>> > >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> > >>> --- > >>> hw/usb/dev-audio.c | 2 +- > >>> hw/usb/dev-mtp.c | 4 ++-- > >>> hw/usb/hcd-ehci.c | 2 +- > >>> 3 files changed, 4 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c > >>> index bfebfe9..988f6cc 100644 > >>> --- a/hw/usb/dev-audio.c > >>> +++ b/hw/usb/dev-audio.c > >>> @@ -371,7 +371,7 @@ static void output_callback(void *opaque, int avail) > >>> return; > >>> } > >>> data = streambuf_get(&s->out.buf); > >>> - if (NULL == data) { > >>> + if (data == NULL) { > >> > >> Wouldn't it be even more idiomatic as: > >> > >> if (!data) { > >> > >> Probably applies throughout your series. > >> > > OK, will do. Thanks! > > Not so quick! You are free to use that in your patches, but please don't > change all code that way without the author's consent. Just like "equals > null" is a natural English way of reading, compared to "null equals > something", "not null" reads like a boolean expression to me, and even > worse while all valid C, "not strcmp" leads to mind-boggling inverted > logic... > OK, I will wait for other maintainer's comments. Thanks! Our focus is just on doing not use 'Yoda conditions' in QEMU. > Regards, > Andreas > > -- > SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany > GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg Best regards, -Gonglei
Andreas Färber <afaerber@suse.de> writes: > Am 01.08.2014 05:32, schrieb Gonglei (Arei): >> Hi, >> >>> Subject: Re: [PATCH v2 2/8] usb: a trivial code change for more >>> idiomatic writing >>> style >>> >>> On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote: >>>> From: Gonglei <arei.gonglei@huawei.com> >>>> >>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>>> --- >>>> hw/usb/dev-audio.c | 2 +- >>>> hw/usb/dev-mtp.c | 4 ++-- >>>> hw/usb/hcd-ehci.c | 2 +- >>>> 3 files changed, 4 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c >>>> index bfebfe9..988f6cc 100644 >>>> --- a/hw/usb/dev-audio.c >>>> +++ b/hw/usb/dev-audio.c >>>> @@ -371,7 +371,7 @@ static void output_callback(void *opaque, int avail) >>>> return; >>>> } >>>> data = streambuf_get(&s->out.buf); >>>> - if (NULL == data) { >>>> + if (data == NULL) { >>> >>> Wouldn't it be even more idiomatic as: >>> >>> if (!data) { >>> >>> Probably applies throughout your series. >>> >> OK, will do. Thanks! > > Not so quick! You are free to use that in your patches, but please don't > change all code that way without the author's consent. Just like "equals > null" is a natural English way of reading, compared to "null equals > something", "not null" reads like a boolean expression to me, and even > worse while all valid C, "not strcmp" leads to mind-boggling inverted > logic... !strcmp() is somewhat error prone, because it suggests inequality. Can't claim that for !data. That one suggests "no data", which is exactly right. Like Eric, I prefer it to the cumbersome data == NULL. data == 0 is right out. Since there's no consensus on !data vs. data == NULL, you're free to follow your own taste in new code. When changing existing code, imitate nearby code. When nearby code is inconsistent, it's your own taste again.
Hi, > Subject: Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more > idiomatic writing style > > Andreas Färber <afaerber@suse.de> writes: > > > Am 01.08.2014 05:32, schrieb Gonglei (Arei): > >> Hi, > >> > >>> Subject: Re: [PATCH v2 2/8] usb: a trivial code change for more > >>> idiomatic writing > >>> style > >>> > >>> On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote: > >>>> From: Gonglei <arei.gonglei@huawei.com> > >>>> > >>>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> > >>>> --- > >>>> hw/usb/dev-audio.c | 2 +- > >>>> hw/usb/dev-mtp.c | 4 ++-- > >>>> hw/usb/hcd-ehci.c | 2 +- > >>>> 3 files changed, 4 insertions(+), 4 deletions(-) > >>>> > >>>> diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c > >>>> index bfebfe9..988f6cc 100644 > >>>> --- a/hw/usb/dev-audio.c > >>>> +++ b/hw/usb/dev-audio.c > >>>> @@ -371,7 +371,7 @@ static void output_callback(void *opaque, int > avail) > >>>> return; > >>>> } > >>>> data = streambuf_get(&s->out.buf); > >>>> - if (NULL == data) { > >>>> + if (data == NULL) { > >>> > >>> Wouldn't it be even more idiomatic as: > >>> > >>> if (!data) { > >>> > >>> Probably applies throughout your series. > >>> > >> OK, will do. Thanks! > > > > Not so quick! You are free to use that in your patches, but please don't > > change all code that way without the author's consent. Just like "equals > > null" is a natural English way of reading, compared to "null equals > > something", "not null" reads like a boolean expression to me, and even > > worse while all valid C, "not strcmp" leads to mind-boggling inverted > > logic... > > !strcmp() is somewhat error prone, because it suggests inequality. > Can't claim that for !data. That one suggests "no data", which is > exactly right. Like Eric, I prefer it to the cumbersome data == NULL. > data == 0 is right out. > > Since there's no consensus on !data vs. data == NULL, you're free to > follow your own taste in new code. Agreed. > When changing existing code, imitate > nearby code. When nearby code is inconsistent, it's your own taste > again. Yes, I think this is a pretty good policy. Thanks! Best Regards, -Gonglei
On 08/01/2014 12:41 AM, Markus Armbruster wrote: >>>>> + if (data == NULL) { >>>> >>>> Wouldn't it be even more idiomatic as: >>>> >>>> if (!data) { >>>> >>>> Probably applies throughout your series. >>>> >>> OK, will do. Thanks! >> >> Not so quick! You are free to use that in your patches, but please don't >> change all code that way without the author's consent. Just like "equals >> null" is a natural English way of reading, compared to "null equals >> something", "not null" reads like a boolean expression to me, and even >> worse while all valid C, "not strcmp" leads to mind-boggling inverted >> logic... If it's going to be controversial, then the right thing to do is that both '(!ptr)' and '(ptr == NULL)' are acceptable, and that preference should be given to consistency to nearby code. > > !strcmp() is somewhat error prone, because it suggests inequality. That's why libvirt uses a STREQ() macro; it evaluates to !strcmp under the hood, but it's a LOT easier to reason about 'STREQ(a, b)' than it is to reason about '!strcmp(a, b)'. > Can't claim that for !data. That one suggests "no data", which is > exactly right. Like Eric, I prefer it to the cumbersome data == NULL. I also prefer 'if (ptr)' over 'if (ptr != NULL)'. > data == 0 is right out. Correct, especially if data is a pointer (hmm, our coding style should mention that we STRONGLY prefer NULL over 0 when referring to the null pointer). > > Since there's no consensus on !data vs. data == NULL, you're free to > follow your own taste in new code. When changing existing code, imitate > nearby code. When nearby code is inconsistent, it's your own taste > again. Yes, so capturing this sentiment in the coding style guide would be worthwhile.
diff --git a/hw/usb/dev-audio.c b/hw/usb/dev-audio.c index bfebfe9..988f6cc 100644 --- a/hw/usb/dev-audio.c +++ b/hw/usb/dev-audio.c @@ -371,7 +371,7 @@ static void output_callback(void *opaque, int avail) return; } data = streambuf_get(&s->out.buf); - if (NULL == data) { + if (data == NULL) { return; } AUD_write(s->out.voice, data, USBAUDIO_PACKET_SIZE); diff --git a/hw/usb/dev-mtp.c b/hw/usb/dev-mtp.c index 384d4a5..0820046 100644 --- a/hw/usb/dev-mtp.c +++ b/hw/usb/dev-mtp.c @@ -832,7 +832,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c) return; } data_in = usb_mtp_get_object(s, c, o); - if (NULL == data_in) { + if (data_in == NULL) { usb_mtp_queue_result(s, RES_GENERAL_ERROR, c->trans, 0, 0, 0); return; @@ -851,7 +851,7 @@ static void usb_mtp_command(MTPState *s, MTPControl *c) return; } data_in = usb_mtp_get_partial_object(s, c, o); - if (NULL == data_in) { + if (data_in == NULL) { usb_mtp_queue_result(s, RES_GENERAL_ERROR, c->trans, 0, 0, 0); return; diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c index a00a93c..448e007 100644 --- a/hw/usb/hcd-ehci.c +++ b/hw/usb/hcd-ehci.c @@ -1596,7 +1596,7 @@ static EHCIQueue *ehci_state_fetchqh(EHCIState *ehci, int async) entry = ehci_get_fetch_addr(ehci, async); q = ehci_find_queue_by_qh(ehci, entry, async); - if (NULL == q) { + if (q == NULL) { q = ehci_alloc_queue(ehci, entry, async); }