[Qemu-devel] [PATCH v2 for-2.2 0/8] about Yoda conditions

[Qemu-devel] [PATCH v2 for-2.2 0/8] about Yoda conditions

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

$WHATEVER: don't use 'Yoda conditions'

'Yoda conditions' are not part of idiomatic QEMU coding
style, so rewrite them in the more usual order.

v2:
 - add more specific commit messages suggested by PMM, thanks.
 - introduce section of conditional statement to CODING_STYLE.


Gonglei (8):
  CODING_STYLE: Section about conditional statement
  usb: a trivial code change for more idiomatic writing style
  audio: a trivial code change for more idiomatic writing style
  isa-bus: a trivial code change for more idiomatic writing style
  a trivial code change for more idiomatic writing style
  spice: a trivial code change for more idiomatic writing style
  vl: a trivial code change for more idiomatic writing style
  vmxnet3: a trivial code change for more idiomatic writing style

 CODING_STYLE         | 19 +++++++++++++++++++
 hw/audio/gus.c       |  2 +-
 hw/audio/hda-codec.c |  3 ++-
 hw/audio/sb16.c      |  6 +++---
 hw/isa/isa-bus.c     |  2 +-
 hw/net/vmxnet3.c     | 16 ++++++++--------
 hw/usb/dev-audio.c   |  2 +-
 hw/usb/dev-mtp.c     |  4 ++--
 hw/usb/hcd-ehci.c    |  2 +-
 qdev-monitor.c       |  2 +-
 qemu-char.c          |  2 +-
 ui/spice-core.c      |  4 ++--
 util/qemu-sockets.c  |  2 +-
 vl.c                 |  5 +++--
 14 files changed, 46 insertions(+), 25 deletions(-)

-- 
1.7.12.4

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

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Yoda conidtions lack of readability, and QEMU have a
strict compiler configuration for checking a common
mistake like "if (dev = NULL)". Make it a written rule.

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 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
+
+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.
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[arei.gonglei]

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) {
             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);
     }
 
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 3/8] audio: a trivial code change for more idiomatic writing style

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/audio/gus.c       | 2 +-
 hw/audio/hda-codec.c | 3 ++-
 hw/audio/sb16.c      | 6 +++---
 3 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/hw/audio/gus.c b/hw/audio/gus.c
index bba6840..4a43ce7 100644
--- a/hw/audio/gus.c
+++ b/hw/audio/gus.c
@@ -212,7 +212,7 @@ static int GUS_read_DMA (void *opaque, int nchan, int dma_pos, int dma_len)
         pos += copied;
     }
 
-    if (0 == ((mode >> 4) & 1)) {
+    if (((mode >> 4) & 1) == 0) {
         DMA_release_DREQ (s->emu.gusdma);
     }
     return dma_len;
diff --git a/hw/audio/hda-codec.c b/hw/audio/hda-codec.c
index cbcf521..3c03ff5 100644
--- a/hw/audio/hda-codec.c
+++ b/hw/audio/hda-codec.c
@@ -489,8 +489,9 @@ static int hda_audio_init(HDACodecDevice *hda, const struct desc_codec *desc)
     for (i = 0; i < a->desc->nnodes; i++) {
         node = a->desc->nodes + i;
         param = hda_codec_find_param(node, AC_PAR_AUDIO_WIDGET_CAP);
-        if (NULL == param)
+        if (param == NULL) {
             continue;
+        }
         type = (param->val & AC_WCAP_TYPE) >> AC_WCAP_TYPE_SHIFT;
         switch (type) {
         case AC_WID_AUD_OUT:
diff --git a/hw/audio/sb16.c b/hw/audio/sb16.c
index 60c4b3b..bda26d0 100644
--- a/hw/audio/sb16.c
+++ b/hw/audio/sb16.c
@@ -928,7 +928,7 @@ static IO_WRITE_PROTO (dsp_write)
 /*         if (s->highspeed) */
 /*             break; */
 
-        if (0 == s->needed_bytes) {
+        if (s->needed_bytes == 0) {
             command (s, val);
 #if 0
             if (0 == s->needed_bytes) {
@@ -1212,7 +1212,7 @@ static int SB_read_DMA (void *opaque, int nchan, int dma_pos, int dma_len)
 #endif
 
     if (till <= copy) {
-        if (0 == s->dma_auto) {
+        if (s->dma_auto == 0) {
             copy = till;
         }
     }
@@ -1224,7 +1224,7 @@ static int SB_read_DMA (void *opaque, int nchan, int dma_pos, int dma_len)
     if (s->left_till_irq <= 0) {
         s->mixer_regs[0x82] |= (nchan & 4) ? 2 : 1;
         qemu_irq_raise (s->pic);
-        if (0 == s->dma_auto) {
+        if (s->dma_auto == 0) {
             control (s, 0);
             speaker (s, 0);
         }
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 4/8] isa-bus: a trivial code change for more idiomatic writing style

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/isa/isa-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/isa/isa-bus.c b/hw/isa/isa-bus.c
index b28981b..851f953 100644
--- a/hw/isa/isa-bus.c
+++ b/hw/isa/isa-bus.c
@@ -50,7 +50,7 @@ ISABus *isa_bus_new(DeviceState *dev, MemoryRegion *address_space_io)
         fprintf(stderr, "Can't create a second ISA bus\n");
         return NULL;
     }
-    if (NULL == dev) {
+    if (dev == NULL) {
         dev = qdev_create(NULL, "isabus-bridge");
         qdev_init_nofail(dev);
     }
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 5/8] a trivial code change for more idiomatic writing style

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 qdev-monitor.c      | 2 +-
 qemu-char.c         | 2 +-
 util/qemu-sockets.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/qdev-monitor.c b/qdev-monitor.c
index f87f3d8..3e30d38 100644
--- a/qdev-monitor.c
+++ b/qdev-monitor.c
@@ -694,7 +694,7 @@ void qmp_device_del(const char *id, Error **errp)
     DeviceState *dev;
 
     dev = qdev_find_recursive(sysbus_get_default(), id);
-    if (NULL == dev) {
+    if (dev == NULL) {
         error_set(errp, QERR_DEVICE_NOT_FOUND, id);
         return;
     }
diff --git a/qemu-char.c b/qemu-char.c
index 956be49..70d5a64 100644
--- a/qemu-char.c
+++ b/qemu-char.c
@@ -4117,7 +4117,7 @@ void qmp_chardev_remove(const char *id, Error **errp)
     CharDriverState *chr;
 
     chr = qemu_chr_find(id);
-    if (NULL == chr) {
+    if (chr == NULL) {
         error_setg(errp, "Chardev '%s' not found", id);
         return;
     }
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 74cf078..5d38395 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -732,7 +732,7 @@ int unix_connect_opts(QemuOpts *opts, Error **errp,
     ConnectState *connect_state = NULL;
     int sock, rc;
 
-    if (NULL == path) {
+    if (path == NULL) {
         error_setg(errp, "unix connect: no path specified");
         return -1;
     }
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 6/8] spice: a trivial code change for more idiomatic writing style

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 ui/spice-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 7bb91e6..ff54dac 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -677,7 +677,7 @@ void qemu_spice_init(void)
 
     if (tls_port) {
         x509_dir = qemu_opt_get(opts, "x509-dir");
-        if (NULL == x509_dir) {
+        if (x509_dir == NULL) {
             x509_dir = ".";
         }
 
@@ -803,7 +803,7 @@ void qemu_spice_init(void)
 
     seamless_migration = qemu_opt_get_bool(opts, "seamless-migration", 0);
     spice_server_set_seamless_migration(spice_server, seamless_migration);
-    if (0 != spice_server_init(spice_server, &core_interface)) {
+    if (spice_server_init(spice_server, &core_interface) != 0) {
         error_report("failed to initialize spice server");
         exit(1);
     };
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 7/8] vl: a trivial code change for more idiomatic writing style

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 vl.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index fe451aa..69f0f20 100644
--- a/vl.c
+++ b/vl.c
@@ -1136,7 +1136,7 @@ static int drive_init_func(QemuOpts *opts, void *opaque)
 
 static int drive_enable_snapshot(QemuOpts *opts, void *opaque)
 {
-    if (NULL == qemu_opt_get(opts, "snapshot")) {
+    if (qemu_opt_get(opts, "snapshot") == NULL) {
         qemu_opt_set(opts, "snapshot", "on");
     }
     return 0;
@@ -2488,8 +2488,9 @@ static int foreach_device_config(int type, int (*func)(const char *cmdline))
         loc_push_restore(&conf->loc);
         rc = func(conf->cmdline);
         loc_pop(&conf->loc);
-        if (0 != rc)
+        if (rc != 0) {
             return rc;
+        }
     }
     return 0;
 }
-- 
1.7.12.4

[Qemu-devel] [PATCH v2 8/8] vmxnet3: a trivial code change for more idiomatic writing style

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

Signed-off-by: Gonglei <arei.gonglei@huawei.com>
---
 hw/net/vmxnet3.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
index 77bea6f..34c5aff 100644
--- a/hw/net/vmxnet3.c
+++ b/hw/net/vmxnet3.c
@@ -1009,7 +1009,7 @@ vmxnet3_indicate_packet(VMXNET3State *s)
 
         vmxnet3_dump_rx_descr(&rxd);
 
-        if (0 != ready_rxcd_pa) {
+        if (ready_rxcd_pa != 0) {
             cpu_physical_memory_write(ready_rxcd_pa, &rxcd, sizeof(rxcd));
         }
 
@@ -1020,7 +1020,7 @@ vmxnet3_indicate_packet(VMXNET3State *s)
         rxcd.gen = new_rxcd_gen;
         rxcd.rqID = RXQ_IDX + rx_ridx * s->rxq_num;
 
-        if (0 == bytes_left) {
+        if (bytes_left == 0) {
             vmxnet3_rx_update_descr(s->rx_pkt, &rxcd);
         }
 
@@ -1038,16 +1038,16 @@ vmxnet3_indicate_packet(VMXNET3State *s)
         num_frags++;
     }
 
-    if (0 != ready_rxcd_pa) {
+    if (ready_rxcd_pa != 0) {
         rxcd.eop = 1;
-        rxcd.err = (0 != bytes_left);
+        rxcd.err = (bytes_left != 0);
         cpu_physical_memory_write(ready_rxcd_pa, &rxcd, sizeof(rxcd));
 
         /* Flush RX descriptor changes */
         smp_wmb();
     }
 
-    if (0 != new_rxcd_pa) {
+    if (new_rxcd_pa != 0) {
         vmxnet3_revert_rxc_descr(s, RXQ_IDX);
     }
 
@@ -1190,8 +1190,8 @@ static void vmxnet3_update_mcast_filters(VMXNET3State *s)
     s->mcast_list_len = list_bytes / sizeof(s->mcast_list[0]);
 
     s->mcast_list = g_realloc(s->mcast_list, list_bytes);
-    if (NULL == s->mcast_list) {
-        if (0 == s->mcast_list_len) {
+    if (s->mcast_list == NULL) {
+        if (s->mcast_list_len == 0) {
             VMW_CFPRN("Current multicast list is empty");
         } else {
             VMW_ERPRN("Failed to allocate multicast list of %d elements",
@@ -1667,7 +1667,7 @@ vmxnet3_io_bar1_write(void *opaque,
          * memory address. We save it to temp variable and set the
          * shared address only after we get the high part
          */
-        if (0 == val) {
+        if (val == 0) {
             s->device_active = false;
         }
         s->temp_shared_guest_driver_memory = val;
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2 for-2.2 0/8] about Yoda conditions

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1251 bytes --]

On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> $WHATEVER: don't use 'Yoda conditions'
> 
> 'Yoda conditions' are not part of idiomatic QEMU coding
> style, so rewrite them in the more usual order.
> 
> v2:
>  - add more specific commit messages suggested by PMM, thanks.
>  - introduce section of conditional statement to CODING_STYLE.
> 
> 
> Gonglei (8):
>   CODING_STYLE: Section about conditional statement
>   usb: a trivial code change for more idiomatic writing style

You didn't rename the subject lines.  I think the request you were given
on v1 was to name this patch:

usb: don't use 'Yoda conditions'

>   audio: a trivial code change for more idiomatic writing style

audio: don't use 'Yoda conditions'

>   isa-bus: a trivial code change for more idiomatic writing style
>   a trivial code change for more idiomatic writing style
>   spice: a trivial code change for more idiomatic writing style
>   vl: a trivial code change for more idiomatic writing style
>   vmxnet3: a trivial code change for more idiomatic writing style

and so on

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2406 bytes --]

On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> Yoda conidtions lack of readability, and QEMU have a

s/conidtions/conditions/
s/of //
s/have/has/

> strict compiler configuration for checking a common
> mistake like "if (dev = NULL)". Make it a written rule.
> 
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> ---
>  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/

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

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

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 894 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2 for-2.2 0/8] about Yoda conditions

[Gonglei (Arei)]

Hi,

> Subject: Re: [PATCH v2 for-2.2 0/8] about Yoda conditions
> 
> On 07/31/2014 08:32 PM, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > $WHATEVER: don't use 'Yoda conditions'
> >
> > 'Yoda conditions' are not part of idiomatic QEMU coding
> > style, so rewrite them in the more usual order.
> >
> > v2:
> >  - add more specific commit messages suggested by PMM, thanks.
> >  - introduce section of conditional statement to CODING_STYLE.
> >
> >
> > Gonglei (8):
> >   CODING_STYLE: Section about conditional statement
> >   usb: a trivial code change for more idiomatic writing style
> 
> You didn't rename the subject lines.  I think the request you were given
> on v1 was to name this patch:
> 
> usb: don't use 'Yoda conditions'
> 
> >   audio: a trivial code change for more idiomatic writing style
> 
> audio: don't use 'Yoda conditions'
> 
> >   isa-bus: a trivial code change for more idiomatic writing style
> >   a trivial code change for more idiomatic writing style
> >   spice: a trivial code change for more idiomatic writing style
> >   vl: a trivial code change for more idiomatic writing style
> >   vmxnet3: a trivial code change for more idiomatic writing style
> 
> and so on
> 
Got it, thanks!

> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

Best regards,
-Gonglei

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, arei.gonglei@huawei.com wrote:
> > From: Gonglei <arei.gonglei@huawei.com>
> >
> > 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 <arei.gonglei@huawei.com>
> > ---
> >  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

Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[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!

> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[Andreas Färber]

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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[Gonglei (Arei)]

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

Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[Markus Armbruster]

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.

Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[Gonglei (Arei)]

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

Re: [Qemu-devel] [PATCH v2 2/8] usb: a trivial code change for more idiomatic writing style

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1906 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]