Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci

[Markus Armbruster]

Thomas Huth <thuth@redhat.com> writes:

> On 21/11/2022 17.32, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> On 21/11/22 15:36, Peter Maydell wrote:
>>>> On Mon, 21 Nov 2022 at 14:03, Markus Armbruster <armbru@redhat.com> wrote:
>>>>>
>>>>> Tweak the semantic patch to drop redundant parenthesis around the
>>>>> return expression.
>>>>>
>>>>> Coccinelle drops comments in hw/rdma/vmw/pvrdma_cmd.c; restored
>>>>> manually.
>>>>>
>>>>> Coccinelle messes up vmdk_co_create(), not sure why.  Transformed
>>>>> manually.
>>>>>
>>>>> Line breaks in target/avr/cpu.h and hw/rdma/vmw/pvrdma_cmd.c tidied up
>>>>> manually.
>>>>>
>>>>> Whitespace in fuse_reply_iov() tidied up manually.
>>>>>
>>>>> checkpatch.pl complains "return of an errno should typically be -ve"
>>>>> two times for hw/9pfs/9p-synth.c.  Preexisting, the patch merely makes
>>>>> it visible to checkpatch.pl.
>>>>>
>>>>> checkpatch.pl complains "return is not a function, parentheses are not
>>>>> required" three times for target/mips/tcg/dsp_helper.c.  False
>>>>> positives.
>>>>>
>>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>>>
>>>>>    .../user/ase/msa/bit-count/test_msa_nloc_b.c  |   9 +-
>>>>>    .../user/ase/msa/bit-count/test_msa_nloc_d.c  |   9 +-
>>>> [snip long list of other mips test files]
>>>>
>>>>>    328 files changed, 989 insertions(+), 2099 deletions(-)
>>>> This patch seems to almost entirely be huge because of these
>>>> mips test case files. Are they specific to QEMU or are they
>>>> effectively a 3rd-party import that it doesn't make sense
>>>> to make local changes to?
>>>
>>> They are imported and will unlikely be modified.
>> 
>> Not obvious to me from git-log.
>> 
>> Should I drop the changes to tests/tcg/mips/?
>
> I'd say yes. At least move them to a separate patch.

Possible status of tests/tcg/mips/:

1. Imported, should not be modified

   Drop from the patch.

2. Not imported, should be modified

2a. To be reviewed separately from the remainder of the patch

    Split off.

2b. Likewise, but nobody will care to review, realistically

    Split off and merge anyway, or drop.  I'd go for the latter.

2c. To be reviewed together with the remainder of the patch

    Keep as is.

Which one is it?

>                                                      Otherwise reviewing 
> this patch here is no fun at all.

I don't think complete detailed review is necessary or even sensible.

Review should start with the Coccinelle script:

    // replace 'R = X; return R;' with 'return X;'
    @@
    identifier VAR;
    expression E;
    type T;
    identifier F;
    @@
     T F(...)
     {
         ...
    -    T VAR;
         ... when != VAR

    -    VAR = (E);
    -    return VAR;
    +    return E;
         ... when != VAR
     }

What could go wrong?  Not a rhetorical question!

The simple part is executing the rule.  It'll *delete* variable VAR, and
*preserve* expression E.

The tricky part is deciding whether the rule matches, in particular
proving that there are no other uses of VAR.  If Coccinelle gets that
wrong, the code no longer compiles *unless* there is another, global VAR
of suitable type.

Turns out Coccinelle does get it wrong for vmdk_co_create(), and the
compiler duly rejects it.  I don't understand why it gets it wrong.

To help understand the script, and as a sanity check, reviewing some of
its patches is advisable.  Reviewing all of them feels impractical.