Re: [PATCH] Fix erroneous double negation in conditional

Re: [PATCH] Fix erroneous double negation in conditional

[Michael S. Tsirkin]

On Tue, Apr 28, 2020 at 12:04:29AM -0400, Raphael Norwitz wrote:
> In vhost_migration_log() there is the following check:
>     if(!!enable == dev->log_enabled) {
>         return 0;
>     }
> 
> The double negative “!!” is unnecessary and bad coding style.

It converts the value to bool.

> This
> change removes it.
> 
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
>  hw/virtio/vhost.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index aff98a0..83be0c8 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -814,7 +814,7 @@ static int vhost_migration_log(MemoryListener *listener, int enable)
>      struct vhost_dev *dev = container_of(listener, struct vhost_dev,
>                                           memory_listener);
>      int r;
> -    if (!!enable == dev->log_enabled) {
> +    if (enable == dev->log_enabled) {
>          return 0;
>      }
>      if (!dev->started) {
> -- 
> 1.8.3.1

[PATCH] Fix erroneous double negation in conditional

[Raphael Norwitz]

In vhost_migration_log() there is the following check:
    if(!!enable == dev->log_enabled) {
        return 0;
    }

The double negative “!!” is unnecessary and bad coding style. This
change removes it.

Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 hw/virtio/vhost.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index aff98a0..83be0c8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -814,7 +814,7 @@ static int vhost_migration_log(MemoryListener
*listener, int enable)
     struct vhost_dev *dev = container_of(listener, struct vhost_dev,
                                          memory_listener);
     int r;
-    if (!!enable == dev->log_enabled) {
+    if (enable == dev->log_enabled) {
         return 0;
     }
     if (!dev->started) {
--
1.8.3.1

Re: [PATCH] Fix erroneous double negation in conditional

[Eric Blake]

On 5/7/20 3:06 PM, Raphael Norwitz wrote:
> In vhost_migration_log() there is the following check:
>      if(!!enable == dev->log_enabled) {
>          return 0;
>      }
> 
> The double negative “!!” is unnecessary and bad coding style. This
> change removes it.

!!int or !!ptr is not bad coding style - it is the shortest way to 
compare a non-bool against 0, and canonicalize the result back into bool 
(that is, convert all non-zero values into '1').  But !!bool is a waste 
of typing, since bool is already in the proper form.  Your patch as-is 
is incorrect; since the function declares 'int enable', this is using 
the !!int form which is not bad coding style.

On the other hand, looking at this function closer, we see that 
vhost_migration_log() is static, so all uses lie within this file.  And 
the callers are:

static void vhost_log_global_start(MemoryListener *listener)
     r = vhost_migration_log(listener, true);
static void vhost_log_global_stop(MemoryListener *listener)
     r = vhost_migration_log(listener, false);

and looking at struct vhost_dev, its log_enabled member is bool.

So the _real_ problem with this file is that it uses 'int enable' rather 
than 'bool enable'.  And once you fix the parameter type, then you are 
indeed correct that you would have a !!bool scenario worth cleaning up.

Looking forward to v2 along those lines.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org