Re: [PATCH v1 00/59] trivial unneeded labels cleanup

[Daniel Henrique Barboza]

On 1/10/20 4:05 PM, Eric Blake wrote:
> On 1/6/20 12:23 PM, Daniel Henrique Barboza wrote:
> > Hello,
> >
> > This is the type of cleanup I've contributed to Libvirt
> > during the last year. Figured QEMU also deserves the same
> > care.
> >
> > The idea here is remove unneeded labels. By 'unneeded' I
> > mean labels that does nothing but a 'return' call. One
> > common case is something like this:
> >
> > if ()
> >      goto cleanup;
> > [...]
> >   cleanup:
> >      return 0;
> >
> > This code can be simplified to:
> >
> > if ()
> >      return 0;
>
> How much of this work is done manually, and how much via Coccinelle?


I'm still getting along with Coccinelle. I'm able to do simple matches but
couldn't make it work to match this scenario. I didn't invest too much
time on it though.


> >   qga/commands-win32.c          | 17 ++++---
> >   target/mips/mips-semi.c       | 15 +++---
> >   target/unicore32/softmmu.c    | 23 +++-------
> >   util/aio-posix.c              |  3 +-
> >   util/module.c                 | 11 ++---
>
> Hmm, no change to scripts/coccinelle, so presumably all manual :(

I used pcregrep:

$ pcregrep --exclude-dir=build --exclude-dir=docs --exclude-dir=scripts -r -n -M 
':\n*\s*return' . > clean_labels

(note: there was a lot more of --exclude-dir in the original command, 
unfortunately I
didn't record it)

Then I filtered in the output file the cases where the regexp matched "switch"
labels and any other false positives like strings in comments. It's not manual
inspection, but it was crude indeed.



> If we have a Coccinelle script that performs this transformation, we are more 
> likely to be able to catch all places in the tree that would benefit (rather 
> than relying on grep calls or other manual inspection), and more importantly, 
> we can repeat the effort periodically to fix future additions that don't comply 
> with the preferred style as well as backport the patch by rerunning the 
> Coccinelle script with less worry of changed context.  Automated cleanups are 
> always going to be easier to swallow (even if the diffstat is larger) than 
> manual ad hoc cleanups.

I am not aware of how QEMU handles Coccinelle, if it is imposed via
./scripts/checkpatch.pl or something that it is ran from time to time to
suggest changes. But if it's desirable, I can see if I can cook a Coccinelle
script (or anything a bit more sophisticated than what I did)  to flag these
cases I attempted to handle with this series.


Thanks,


DHB


>