Re: [Qemu-devel] [RFC v2 3/3] Remove unnecessary variables for function return value

[Eric Blake <eblake@redhat.com>]

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

On 06/10/2016 02:12 PM, Eduardo Habkost wrote:
> Use Coccinelle script to replace 'ret = E; return ret' with
> 'return E'. The script will do the substitution only when the
> function return type and variable type are the same.
> 
> Sending as RFC because the patch looks more intrusive than the
> others. Probably better to split it per subsystem and let each
> maintainer review and apply it?

Borderline on size, so yeah, splitting it across several subsystems may
ease review (although then the patch will be committed in piecemeal
fashion, and you'd have to ensure the script/coccinelle/ patch goes in
first...)

At any rate, it's fairly mechanical, so I'll review it as is:

> 
> Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
> ---

>  47 files changed, 90 insertions(+), 254 deletions(-)
>  create mode 100644 scripts/coccinelle/return_directly.cocci

Nice diffstat.

> +++ b/block/qcow2-cluster.c
> @@ -154,11 +154,8 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
>      uint64_t **l2_table)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -    int ret;
> -
> -    ret = qcow2_cache_get(bs, s->l2_table_cache, l2_offset, (void**) l2_table);
> -
> -    return ret;
> +    return qcow2_cache_get(bs, s->l2_table_cache, l2_offset,
> +                           (void **)l2_table);

Coccinelle changed spacing of the cast. I don't care strongly enough to
require a touchup if this is the only thing, but may be worth fixing if
you have to respin (for example to split up by submaintainers).

> +++ b/block/raw_bsd.c
> @@ -190,10 +190,7 @@ static int raw_has_zero_init(BlockDriverState *bs)
>  
>  static int raw_create(const char *filename, QemuOpts *opts, Error **errp)
>  {
> -    int ret;
> -
> -    ret = bdrv_create_file(filename, opts, errp);
> -    return ret;
> +    return bdrv_create_file(filename, opts, errp);
>  }

Potential followup patch: delete raw_create(), and:
- .bdrv_create = &raw_create,
+ .bdrv_create = bdrv_create_file,

but doesn't affect this patch.

> +++ b/block/rbd.c
> @@ -875,10 +875,7 @@ static int qemu_rbd_snap_rollback(BlockDriverState *bs,
>                                    const char *snapshot_name)
>  {
>      BDRVRBDState *s = bs->opaque;
> -    int r;
> -
> -    r = rbd_snap_rollback(s->image, snapshot_name);
> -    return r;
> +    return rbd_snap_rollback(s->image, snapshot_name);

Coccinelle lost the blank line between declarations and statements;
might be nice to manually touch that up and add it back in.

> +++ b/hw/ppc/spapr_vio.c
> @@ -57,12 +57,7 @@ static char *spapr_vio_get_dev_name(DeviceState *qdev)
>  {
>      VIOsPAPRDevice *dev = VIO_SPAPR_DEVICE(qdev);
>      VIOsPAPRDeviceClass *pc = VIO_SPAPR_DEVICE_GET_CLASS(dev);
> -    char *name;
> -
> -    /* Device tree style name device@reg */
> -    name = g_strdup_printf("%s@%x", pc->dt_name, dev->reg);
> -
> -    return name;
> +    return g_strdup_printf("%s@%x", pc->dt_name, dev->reg);

Coccinelle lost the comment; might be worth keeping it.

> +++ b/hw/scsi/megasas.c
> @@ -410,17 +410,9 @@ static void megasas_encode_lba(uint8_t *cdb, uint64_t lba,
>  static uint64_t megasas_fw_time(void)
>  {
>      struct tm curtime;
> -    uint64_t bcd_time;
>  
>      qemu_get_timedate(&curtime, 0);
> -    bcd_time = ((uint64_t)curtime.tm_sec & 0xff) << 48 |
> -        ((uint64_t)curtime.tm_min & 0xff)  << 40 |
> -        ((uint64_t)curtime.tm_hour & 0xff) << 32 |
> -        ((uint64_t)curtime.tm_mday & 0xff) << 24 |
> -        ((uint64_t)curtime.tm_mon & 0xff)  << 16 |
> -        ((uint64_t)(curtime.tm_year + 1900) & 0xffff);
> -
> -    return bcd_time;
> +    return ((uint64_t)curtime.tm_sec & 0xff) << 48 | ((uint64_t)curtime.tm_min & 0xff) << 40 | ((uint64_t)curtime.tm_hour & 0xff) << 32 | ((uint64_t)curtime.tm_mday & 0xff) << 24 | ((uint64_t)curtime.tm_mon & 0xff) << 16 | ((uint64_t)(curtime.tm_year + 1900) & 0xffff);

Eww. Coccinelle botched that formatting.  You'll need to manually fix
this one.

> +++ b/hw/timer/mc146818rtc.c
> @@ -105,12 +105,9 @@ static inline bool rtc_running(RTCState *s)
>  
>  static uint64_t get_guest_rtc_ns(RTCState *s)
>  {
> -    uint64_t guest_rtc;
>      uint64_t guest_clock = qemu_clock_get_ns(rtc_clock);
>  
> -    guest_rtc = s->base_rtc * NANOSECONDS_PER_SECOND +
> -        guest_clock - s->last_update + s->offset;
> -    return guest_rtc;
> +    return s->base_rtc * NANOSECONDS_PER_SECOND + guest_clock - s->last_update + s->offset;
>  }

Worth wrapping that line again (not as bad as the megasas one, though).

> +++ b/qga/commands-win32.c
> @@ -1150,7 +1150,6 @@ out:
>  int64_t qmp_guest_get_time(Error **errp)
>  {
>      SYSTEMTIME ts = {0};
> -    int64_t time_ns;
>      FILETIME tf;
>  
>      GetSystemTime(&ts);
> @@ -1164,10 +1163,7 @@ int64_t qmp_guest_get_time(Error **errp)
>          return -1;
>      }
>  
> -    time_ns = ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime)
> -                - W32_FT_OFFSET) * 100;
> -
> -    return time_ns;
> +    return ((((int64_t)tf.dwHighDateTime << 32) | tf.dwLowDateTime) - W32_FT_OFFSET) * 100;

and again

> +++ b/scripts/coccinelle/return_directly.cocci
> @@ -0,0 +1,19 @@
> +// replace 'R = X; return R;' with 'return R;'
> +
> +// remove assignment
> +@ removal @
> +identifier VAR;
> +expression E;
> +type T;
> +identifier F;
> +@@
> + T F(...)
> + {
> +     ...
> +-    T VAR;
> +     ... when != VAR
> +-    VAR = E;
> +-    return VAR;
> ++    return E;
> +     ... when != VAR
> + }

Looks reasonable; I like that you constrained the type to be the same
(so that we aren't having to also worry about promotion rules while
reviewing it).

> +++ b/target-tricore/op_helper.c
> @@ -2117,7 +2117,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2)
>      int32_t eq_pos = x_sign & ((r1 >> 32) == r2);
>      int32_t eq_neg = x_sign & ((r1 >> 32) == -r2);
>      uint32_t quotient;
> -    uint64_t ret, remainder;
> +    uint64_t remainder;
>  
>      if ((q_sign & ~eq_neg) | eq_pos) {
>          quotient = (r1 + 1) & 0xffffffff;
> @@ -2130,8 +2130,7 @@ uint64_t helper_dvadj(uint64_t r1, uint32_t r2)
>      } else {
>          remainder = (r1 & 0xffffffff00000000ull);
>      }
> -    ret = remainder|quotient;
> -    return ret;
> +    return remainder | quotient;

Coccinelle fixed a checkpatch violation :)

Minor tweaks, and your idea of splitting may be worth while, but all the
changes look correct, so:

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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: 604 bytes --]