Re: [Qemu-devel] [PATCH v3 3/3] coccinelle: Remove unnecessary variables for function return value

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 01:20:14PM +0200, Markus Armbruster wrote:
> Paolo Bonzini  writes:
> 
> > On 14/06/2016 10:57, Markus Armbruster wrote:
> >>> diff --git a/scripts/coccinelle/return_directly.cocci 
> >>> b/scripts/coccinelle/return_directly.cocci
> >>> > new file mode 100644
> >>> > index 000..c52f4fc
> >>> > --- /dev/null
> >>> > +++ b/scripts/coccinelle/return_directly.cocci
> >>> > @@ -0,0 +1,21 @@
> >>> > +// replace 'R = X; return R;' with 'return R;'
> >>> > +
> >>> > +// remove assignment
> >> Second comment feels redundant.  Can drop on commit to error-next.
> >> 
> >>> > +@ removal @
> >> Rule name "removal" is not used.  Can drop on commit to error-next.
> >> 
> >
> > I've seen rule names used as a comment.  Feels a bit like COBOL, but it
> > doesn't hurt.  Perhaps rename it to "@ return_directly @"?
> 
> No objection.  However, the existing semantic patches in
> scripts/coccinelle/ don't do that.  You guys tell me what to do with
> this one.

I think rule names are just noise if the script has only a single
transformation. In this specific case, the comment and the rule
name were kept by mistake.

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 3/3] coccinelle: Remove unnecessary variables for function return value

[Eduardo Habkost]

On Tue, Jun 14, 2016 at 10:57:44AM +0200, Markus Armbruster wrote:
> Eduardo Habkost  writes:
> 
> > 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?
> 
> I guess you forgot to drop this paragraph.  Can do it on commit to
> error-next.

Oops!

> 
> > Manual fixups:
> >
> > * audio/audio.c: coding style of "read (...)" and "write (...)"
> > * block/qcow2-cluster.c: wrap line to make it shorter
> > * block/qcow2-refcount.c: change indentation of wrapped line
> > * target-tricore/op_helper.c: fix coding style of
> >   "remainder|quotient"
> > * target-mips/dsp_helper.c: reverted changes because I don't
> >   want to argue about checkpatch.pl
> > * ui/qemu-pixman.c: fix line indentation
> > * block/rbd.c: restore blank line between declarations and
> >   statements
> >
> > Reviewed-by: Eric Blake 
> > Signed-off-by: Eduardo Habkost 
> [...]
> > diff --git a/scripts/coccinelle/return_directly.cocci 
> > b/scripts/coccinelle/return_directly.cocci
> > new file mode 100644
> > index 000..c52f4fc
> > --- /dev/null
> > +++ b/scripts/coccinelle/return_directly.cocci
> > @@ -0,0 +1,21 @@
> > +// replace 'R = X; return R;' with 'return R;'
> > +
> > +// remove assignment
> 
> Second comment feels redundant.  Can drop on commit to error-next.
> 
> > +@ removal @
> 
> Rule name "removal" is not used.  Can drop on commit to error-next.

Oops, both are leftovers from when I was trying to do it in two
different transformations for some reason. Can be removed.
Thanks!

-- 
Eduardo

Re: [Qemu-devel] [PATCH v3 3/3] coccinelle: Remove unnecessary variables for function return value

[Markus Armbruster]

Paolo Bonzini  writes:

> On 14/06/2016 10:57, Markus Armbruster wrote:
>>> diff --git a/scripts/coccinelle/return_directly.cocci 
>>> b/scripts/coccinelle/return_directly.cocci
>>> > new file mode 100644
>>> > index 000..c52f4fc
>>> > --- /dev/null
>>> > +++ b/scripts/coccinelle/return_directly.cocci
>>> > @@ -0,0 +1,21 @@
>>> > +// replace 'R = X; return R;' with 'return R;'
>>> > +
>>> > +// remove assignment
>> Second comment feels redundant.  Can drop on commit to error-next.
>> 
>>> > +@ removal @
>> Rule name "removal" is not used.  Can drop on commit to error-next.
>> 
>
> I've seen rule names used as a comment.  Feels a bit like COBOL, but it
> doesn't hurt.  Perhaps rename it to "@ return_directly @"?

No objection.  However, the existing semantic patches in
scripts/coccinelle/ don't do that.  You guys tell me what to do with
this one.

Re: [Qemu-devel] [PATCH v3 3/3] coccinelle: Remove unnecessary variables for function return value

[Paolo Bonzini]

On 14/06/2016 10:57, Markus Armbruster wrote:
>> diff --git a/scripts/coccinelle/return_directly.cocci 
>> b/scripts/coccinelle/return_directly.cocci
>> > new file mode 100644
>> > index 000..c52f4fc
>> > --- /dev/null
>> > +++ b/scripts/coccinelle/return_directly.cocci
>> > @@ -0,0 +1,21 @@
>> > +// replace 'R = X; return R;' with 'return R;'
>> > +
>> > +// remove assignment
> Second comment feels redundant.  Can drop on commit to error-next.
> 
>> > +@ removal @
> Rule name "removal" is not used.  Can drop on commit to error-next.
> 

I've seen rule names used as a comment.  Feels a bit like COBOL, but it
doesn't hurt.  Perhaps rename it to "@ return_directly @"?

Paolo

Re: [Qemu-devel] [PATCH v3 3/3] coccinelle: Remove unnecessary variables for function return value

[Markus Armbruster]

Eduardo Habkost  writes:

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

I guess you forgot to drop this paragraph.  Can do it on commit to
error-next.

> Manual fixups:
>
> * audio/audio.c: coding style of "read (...)" and "write (...)"
> * block/qcow2-cluster.c: wrap line to make it shorter
> * block/qcow2-refcount.c: change indentation of wrapped line
> * target-tricore/op_helper.c: fix coding style of
>   "remainder|quotient"
> * target-mips/dsp_helper.c: reverted changes because I don't
>   want to argue about checkpatch.pl
> * ui/qemu-pixman.c: fix line indentation
> * block/rbd.c: restore blank line between declarations and
>   statements
>
> Reviewed-by: Eric Blake 
> Signed-off-by: Eduardo Habkost 
[...]
> diff --git a/scripts/coccinelle/return_directly.cocci 
> b/scripts/coccinelle/return_directly.cocci
> new file mode 100644
> index 000..c52f4fc
> --- /dev/null
> +++ b/scripts/coccinelle/return_directly.cocci
> @@ -0,0 +1,21 @@
> +// replace 'R = X; return R;' with 'return R;'
> +
> +// remove assignment

Second comment feels redundant.  Can drop on commit to error-next.

> +@ removal @

Rule name "removal" is not used.  Can drop on commit to error-next.

> +identifier VAR;
> +expression E;
> +type T;
> +identifier F;
> +@@
> + T F(...)
> + {
> + ...
> +-T VAR;
> + ... when != VAR
> +
> +-VAR =
> ++return
> + E;
> +-return VAR;
> + ... when != VAR
> + }
[...]
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index b04bfaf..afc8d6b 100644
> --- a/block/qcow2-cluster.c
> +++ 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;
> -

I'd prefer to keep this blank line.  Can touch up on commit to error-next.

> -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);
>  }
>  
>  /*
[...]

[Qemu-devel] [PATCH v3 3/3] coccinelle: Remove unnecessary variables for function return value

[Eduardo Habkost]

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?

Manual fixups:

* audio/audio.c: coding style of "read (...)" and "write (...)"
* block/qcow2-cluster.c: wrap line to make it shorter
* block/qcow2-refcount.c: change indentation of wrapped line
* target-tricore/op_helper.c: fix coding style of
  "remainder|quotient"
* target-mips/dsp_helper.c: reverted changes because I don't
  want to argue about checkpatch.pl
* ui/qemu-pixman.c: fix line indentation
* block/rbd.c: restore blank line between declarations and
  statements

Reviewed-by: Eric Blake 
Signed-off-by: Eduardo Habkost 
---
Changes v1 -> v2:
* (new patch)

Changes v2 -> v3:
* Not a RFC anymore
* Used --keep-comments option
* Instead of using:
- VAR = E;
+ return E;
  use:
- VAR =
+ return
  E
  This makes Coccinelle keep the existing formatting on some
  cases.
* Manual fixups
---
 scripts/coccinelle/return_directly.cocci | 21 +
 audio/audio.c| 10 ++
 block/archipelago.c  |  4 +---
 block/qcow2-cluster.c|  7 ++-
 block/qcow2-refcount.c   |  7 ++-
 block/raw-posix.c|  8 ++--
 block/raw_bsd.c  |  5 +
 block/rbd.c  |  4 +---
 block/vmdk.c |  6 ++
 block/vvfat.c|  5 +
 hw/acpi/aml-build.c  | 13 +++--
 hw/audio/intel-hda.c |  5 +
 hw/display/vga.c |  4 +---
 hw/intc/s390_flic_kvm.c  |  5 ++---
 hw/pci-host/uninorth.c   |  5 +
 hw/ppc/spapr_vio.c   |  5 +
 hw/scsi/megasas.c|  5 +
 hw/scsi/scsi-generic.c   |  5 +
 hw/timer/mc146818rtc.c   |  4 +---
 hw/virtio/virtio-pci.c   |  4 +---
 linux-user/signal.c  | 15 ---
 page_cache.c |  5 +
 qga/commands-posix.c |  4 +---
 qga/commands-win32.c |  5 +
 qobject/qlist.c  |  5 +
 target-i386/fpu_helper.c | 10 ++
 target-i386/kvm.c|  5 ++---
 target-mips/op_helper.c  |  4 +---
 target-s390x/helper.c|  6 +-
 target-sparc/cc_helper.c | 25 +
 target-tricore/op_helper.c   | 13 -
 tests/display-vga-test.c |  6 +-
 tests/endianness-test.c  |  5 +
 tests/i440fx-test.c  |  4 +---
 tests/intel-hda-test.c   |  6 +-
 tests/test-filter-redirector.c   |  6 +-
 tests/virtio-blk-test.c  |  5 +
 tests/virtio-console-test.c  |  6 +-
 tests/virtio-net-test.c  |  6 +-
 tests/virtio-scsi-test.c |  6 +-
 tests/wdt_ib700-test.c   |  6 +-
 ui/cursor.c  | 10 ++
 ui/qemu-pixman.c | 13 +
 util/module.c|  6 +-
 vl.c |  5 +
 45 files changed, 90 insertions(+), 229 deletions(-)
 create mode 100644 scripts/coccinelle/return_directly.cocci

diff --git a/scripts/coccinelle/return_directly.cocci 
b/scripts/coccinelle/return_directly.cocci
new file mode 100644
index 000..c52f4fc
--- /dev/null
+++ b/scripts/coccinelle/return_directly.cocci
@@ -0,0 +1,21 @@
+// 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 =
++return
+ E;
+-return VAR;
+ ... when != VAR
+ }
diff --git a/audio/audio.c b/audio/audio.c
index e60c124..9d4dcc7 100644
--- a/audio/audio.c
+++ b/audio/audio.c
@@ -1131,8 +1131,6 @@ static void audio_timer (void *opaque)
  */
 int AUD_write (SWVoiceOut *sw, void *buf, int size)
 {
-int bytes;
-
 if (!sw) {
 /* XXX: Consider options */
 return size;
@@ -1143,14 +1141,11 @@ int AUD_write (SWVoiceOut *sw, void *buf, int size)
 return 0;
 }
 
-bytes = sw->hw->pcm_ops->write (sw, buf, size);
-return bytes;
+return sw->hw->pcm_ops->write(sw, buf, size);
 }
 
 int AUD_read (SWVoiceIn *sw, void *buf, int size)
 {
-int bytes;
-
 if (!sw) {
 /* XXX: Consider options */
 return