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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1890 bytes --]

On 06.01.20 19:23, 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;
> 
> 
> Which is cleaner and requires less brain cycles to wonder
> whether the 'cleanup' label does anything special, such
> as a heap memory cleanup.

For me, it doesn’t require any brain cycles, because I generally just
assume the cleanup label will do the right thing.  OTOH, a return
statement may make me invest some some brain cycles, because maybe there
is something to be cleaned up and it isn’t done here.

> Another common case uses a variable to set a return value,
> generally an error, then return:
> 
> if () {
>     ret = -ENOENT;
>     goto out;
> }
> [..]
>  out:
>     return ret;
> 
> Likewise, it is clearer to just 'return -ENOENT' instead of
> jumping to a label. There are other cases being handled in
> these patches, but these are the most common.

I find it clearer from the perspective of “less LoC”, but I find it less
clear from the perspective of “Is this the right way to clean up?”.

Even on patch 15 (which you say isn’t too much of a debate), I don’t
find the change to make things any clearer.  Just less verbose.

I suppose none of this would matter if we used __attribute__((cleanup))
everywhere and simply never had to clean up anything manually.  But as
long as we don’t and require cleanup paths in many places, I disagree
that they require more brain cycles than plain return statements.

Max


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