Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On Tue, 22 Nov 2022 at 15:04, Philippe Mathieu-Daudé wrote: > > On 21/11/22 17:42, Max Filippov wrote: > > On Mon, Nov 21, 2022 at 6:01 AM Markus Armbruster wrote: > >> .../xtensa/core-dsp3400/xtensa-modules.c.inc | 136 +- > >> target/xtensa/core-lx106/xtensa-modules.c.inc | 16 +-- > > > > These files are generated and were imported from xtensa configuration > > overlays, they're not supposed to be changed. > > Tools can get the repository file list using 'git ls-files', which > itself support file pattern exclusion [*]. > > We can create i.e. 'scripts/imported-files.txt' with: > >linux-headers/ >target/hexagon/imported/ >target/xtensa/core* >tests/tcg/mips/user/ Good idea. scripts/clean-header-guards.pl also has these to add to the exclude list: include/standard-headers/ pc-bios/ tests/tcg/ tests/multiboot/ thanks -- PMM
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On 21/11/22 17:42, Max Filippov wrote: On Mon, Nov 21, 2022 at 6:01 AM Markus Armbruster wrote: .../xtensa/core-dsp3400/xtensa-modules.c.inc | 136 +- target/xtensa/core-lx106/xtensa-modules.c.inc | 16 +-- These files are generated and were imported from xtensa configuration overlays, they're not supposed to be changed. Tools can get the repository file list using 'git ls-files', which itself support file pattern exclusion [*]. We can create i.e. 'scripts/imported-files.txt' with: linux-headers/ target/hexagon/imported/ target/xtensa/core* tests/tcg/mips/user/ Then use 'git ls-files --exclude-from=scripts/imported-files.txt' ... [*] https://git-scm.com/docs/git-ls-files#Documentation/git-ls-files.txt---exclude-fromltfilegt
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On 22/11/22 09:58, Markus Armbruster wrote: Thomas Huth writes: On 21/11/2022 17.32, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: On 21/11/22 15:36, Peter Maydell wrote: On Mon, 21 Nov 2022 at 14:03, Markus Armbruster 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 .../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? "1. Imported, should not be modified" please :)
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On Tue, 22 Nov 2022 at 13:27, Markus Armbruster wrote: > > Peter Maydell writes: > > The obvious answer is "you might have got your manual tweaking > > wrong". A purely mechanised patch I can review by looking at > > the script and maybe eyeballing a few instances of the change; > > a change that is 99% mechanised and 1% hand-written I need to > > run through to find the hand-written parts. > > Define "handwritten" :) > > If reverting unwanted line-breaks and blank lines counts, then I can > make two patches, one straight from Coccinelle, and one that reverts the > unwanted crap. The first one will be larger and more annoying to review > than this one. A clear loss in my book, but I'm the patch submitter, > not a patch reviewer, so my book doesn't matter. > > Else, we're down to one file, which I already offered to split off. > > > But mostly this patch is hard to review for its sheer size, > > mechanical changes or not. A 3000 line patchmail is so big that > > the UI on my mail client gets pretty unwieldy. > > With the manual one split off, target/xtensa/ dropped as requested by > Max, and tests/tcg/mips/ dropped because its status is unclear (and I > start to find it hard to care), we're down to > > 28 files changed, 81 insertions(+), 221 deletions(-) Yes, this is much better and "I hand tweaked these things" is reasonable in a patch that big. It's the combination of the ginormous multi-thousand-line patch and the hand tweaking that was the really awkward part. thanks -- PMM
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
Peter Maydell writes:
> On Tue, 22 Nov 2022 at 08:58, Markus Armbruster wrote:
>> 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 obvious answer is "you might have got your manual tweaking
> wrong". A purely mechanised patch I can review by looking at
> the script and maybe eyeballing a few instances of the change;
> a change that is 99% mechanised and 1% hand-written I need to
> run through to find the hand-written parts.
Define "handwritten" :)
If reverting unwanted line-breaks and blank lines counts, then I can
make two patches, one straight from Coccinelle, and one that reverts the
unwanted crap. The first one will be larger and more annoying to review
than this one. A clear loss in my book, but I'm the patch submitter,
not a patch reviewer, so my book doesn't matter.
Else, we're down to one file, which I already offered to split off.
> But mostly this patch is hard to review for its sheer size,
> mechanical changes or not. A 3000 line patchmail is so big that
> the UI on my mail client gets pretty unwieldy.
With the manual one split off, target/xtensa/ dropped as requested by
Max, and tests/tcg/mips/ dropped because its status is unclear (and I
start to find it hard to care), we're down to
28 files changed, 81 insertions(+), 221 deletions(-)
This will be v2.
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On Tue, 22 Nov 2022 at 08:58, Markus Armbruster wrote:
> 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 obvious answer is "you might have got your manual tweaking
wrong". A purely mechanised patch I can review by looking at
the script and maybe eyeballing a few instances of the change;
a change that is 99% mechanised and 1% hand-written I need to
run through to find the hand-written parts.
But mostly this patch is hard to review for its sheer size,
mechanical changes or not. A 3000 line patchmail is so big that
the UI on my mail client gets pretty unwieldy.
-- PMM
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
Thomas Huth writes:
> On 21/11/2022 17.32, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> On 21/11/22 15:36, Peter Maydell wrote:
On Mon, 21 Nov 2022 at 14:03, Markus Armbruster 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
>.../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.
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
Max Filippov writes: > On Mon, Nov 21, 2022 at 6:01 AM Markus Armbruster wrote: >> .../xtensa/core-dsp3400/xtensa-modules.c.inc | 136 +- >> target/xtensa/core-lx106/xtensa-modules.c.inc | 16 +-- > > These files are generated and were imported from xtensa configuration > overlays, they're not supposed to be changed. Will drop. Thanks!
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On Mon, Nov 21, 2022 at 6:01 AM Markus Armbruster wrote: > .../xtensa/core-dsp3400/xtensa-modules.c.inc | 136 +- > target/xtensa/core-lx106/xtensa-modules.c.inc | 16 +-- These files are generated and were imported from xtensa configuration overlays, they're not supposed to be changed. -- Thanks. -- Max
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On 21/11/2022 17.32, Markus Armbruster wrote: Philippe Mathieu-Daudé writes: On 21/11/22 15:36, Peter Maydell wrote: On Mon, 21 Nov 2022 at 14:03, Markus Armbruster 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 .../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. Otherwise reviewing this patch here is no fun at all. Thomas
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
Philippe Mathieu-Daudé writes: > On 21/11/22 15:36, Peter Maydell wrote: >> On Mon, 21 Nov 2022 at 14:03, Markus Armbruster 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 >> >>> .../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/?
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On 21/11/22 15:36, Peter Maydell wrote: On Mon, 21 Nov 2022 at 14:03, Markus Armbruster 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 .../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.
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
Peter Maydell writes: > On Mon, 21 Nov 2022 at 14:03, Markus Armbruster 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 > >> .../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? Good question. > I definitely don't think you should put manual tidying changes > in the same patch as 3000 lines of automated changes: that > is in my opinion un-reviewable. I can split off the manual cleanup of vmdk_co_create(). The others are really just "don't delete this comment", "don't insert a line containing just spaces", and "don't reflow this expression, keep the line breaks right where they are".
Re: [PATCH] cleanup: Tweak and re-run return_directly.cocci
On Mon, 21 Nov 2022 at 14:03, Markus Armbruster 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 > .../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? I definitely don't think you should put manual tidying changes in the same patch as 3000 lines of automated changes: that is in my opinion un-reviewable. thanks -- PMM
