Message ID | 20180322161226.29796-1-lvivier@redhat.com |
---|---|
Headers | show |
Series | coccinelle: re-run scripts from scripts/coccinelle | expand |
On 03/22/2018 11:12 AM, Laurent Vivier wrote: > I've re-run some scripts from the coccinelle directory, > and they have found some problems. > > This series fixes them. > > Laurent Vivier (4): > error: Strip trailing '\n' from error string arguments (again again) This is user-visible, so appropriate during freeze as a bug fix. It's closes to the error/qapi code that Markus normally maintains (and where I'm filling in while he is out), so I'll queue this one on my QAPI tree. > error: Remove NULL checks on error_propagate() calls > qdict: remove useless cast > Remove unnecessary variables for function return value These are cosmetic; they should be no-ops. 2 and 3 are again best through error/qapi, while 4 is more generic, but there's little reason to split up the series just because 4 is not as closely related. Unless anyone has an opinion otherwise, I'm planning to also include them in my QAPI tree for 2.12.
On 03/22/2018 11:12 AM, Laurent Vivier wrote: > I've re-run some scripts from the coccinelle directory, > and they have found some problems. > > This series fixes them. > > Laurent Vivier (4): > error: Strip trailing '\n' from error string arguments (again again) > error: Remove NULL checks on error_propagate() calls > qdict: remove useless cast > Remove unnecessary variables for function return value I've gone through all the patches to double-check for no surprises; and found some formatting nits on 4 that can be touched up on pull request. Series: Reviewed-by: Eric Blake <eblake@redhat.com>
Hi, This series seems to have some coding style problems. See output below for more information: Type: series Message-id: 20180322161226.29796-1-lvivier@redhat.com Subject: [Qemu-devel] [PATCH 0/4] coccinelle: re-run scripts from scripts/coccinelle === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 git config --local diff.renamelimit 0 git config --local diff.renames True git config --local diff.algorithm histogram commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu t [tag update] patchew/20180321144107.22770-1-kwolf@redhat.com -> patchew/20180321144107.22770-1-kwolf@redhat.com Switched to a new branch 'test' a1c7275b57 Remove unnecessary variables for function return value a530fa5a72 qdict: remove useless cast c1bfe26b11 error: Remove NULL checks on error_propagate() calls db3d55e58e error: Strip trailing '\n' from error string arguments (again again) === OUTPUT BEGIN === Checking PATCH 1/4: error: Strip trailing '\n' from error string arguments (again again)... Checking PATCH 2/4: error: Remove NULL checks on error_propagate() calls... Checking PATCH 3/4: qdict: remove useless cast... Checking PATCH 4/4: Remove unnecessary variables for function return value... ERROR: return is not a function, parentheses are not required #251: FILE: target/mips/dsp_helper.c:3281: + return (temp[1] << 63) | (temp[0] >> 1); ERROR: return is not a function, parentheses are not required #270: FILE: target/mips/dsp_helper.c:3308: + return (temp[1] << 63) | (temp[0] >> 1); ERROR: return is not a function, parentheses are not required #289: FILE: target/mips/dsp_helper.c:3341: + return (temp[1] << 63) | (temp[0] >> 1); total: 3 errors, 0 warnings, 813 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org
On 22 March 2018 at 17:57, <no-reply@patchew.org> wrote: > Checking PATCH 4/4: Remove unnecessary variables for function return value... > ERROR: return is not a function, parentheses are not required > #251: FILE: target/mips/dsp_helper.c:3281: > + return (temp[1] << 63) | (temp[0] >> 1); This looks like a bug in checkpatch. I guess to fix it you'd need to make checkpatch count opening and closing parens in the line to see if it goes to 0 somewhere other than just before the ';'... thanks -- PMM
On 03/22/2018 01:00 PM, Peter Maydell wrote: > On 22 March 2018 at 17:57, <no-reply@patchew.org> wrote: >> Checking PATCH 4/4: Remove unnecessary variables for function return value... >> ERROR: return is not a function, parentheses are not required >> #251: FILE: target/mips/dsp_helper.c:3281: >> + return (temp[1] << 63) | (temp[0] >> 1); > > This looks like a bug in checkpatch. I guess to fix it you'd need > to make checkpatch count opening and closing parens in the line > to see if it goes to 0 somewhere other than just before the ';'... Or if we don't patch the false negative, you can bypass checkpatch with an ugly hack: return 0 + (...) | (...); (I'm NOT going to do that bypass - it's too ugly for my taste)
On 22 March 2018 at 19:12, Eric Blake <eblake@redhat.com> wrote: > Or if we don't patch the false negative, you can bypass checkpatch with an > ugly hack: > > return 0 + (...) | (...); > > (I'm NOT going to do that bypass - it's too ugly for my taste) Yeah, that's definitely not something we should be doing. checkpatch has plenty of false positives anyway, ignoring one more is no big deal. thanks -- PMM
22.03.2018 19:12, Laurent Vivier wrote: > I've re-run some scripts from the coccinelle directory, > and they have found some problems. > > This series fixes them. > > Laurent Vivier (4): > error: Strip trailing '\n' from error string arguments (again again) > error: Remove NULL checks on error_propagate() calls > qdict: remove useless cast > Remove unnecessary variables for function return value I've applied patches 1-3, but the 4th patch is a bit.. interesting. As has already been said, it touches auto-generated files, which is sort of fine, but it _also_ removes comments, which - I think - is quite a bit wrong. I can apply just selected hunks, but.. hmm.. /mjt
On 23/03/2018 13:37, Michael Tokarev wrote: > 22.03.2018 19:12, Laurent Vivier wrote: >> I've re-run some scripts from the coccinelle directory, >> and they have found some problems. >> >> This series fixes them. >> >> Laurent Vivier (4): >> error: Strip trailing '\n' from error string arguments (again again) >> error: Remove NULL checks on error_propagate() calls >> qdict: remove useless cast >> Remove unnecessary variables for function return value > > I've applied patches 1-3, but the 4th patch is a bit.. interesting. > As has already been said, it touches auto-generated files, which is > sort of fine, but it _also_ removes comments, which - I think - is > quite a bit wrong. > > I can apply just selected hunks, but.. hmm.. Thank you, I'm going to rework this one. Thanks, Laurent
On 03/23/2018 07:37 AM, Michael Tokarev wrote: > 22.03.2018 19:12, Laurent Vivier wrote: >> I've re-run some scripts from the coccinelle directory, >> and they have found some problems. >> >> This series fixes them. >> >> Laurent Vivier (4): >> error: Strip trailing '\n' from error string arguments (again again) >> error: Remove NULL checks on error_propagate() calls >> qdict: remove useless cast >> Remove unnecessary variables for function return value > > I've applied patches 1-3, but the 4th patch is a bit.. interesting. > As has already been said, it touches auto-generated files, which is > sort of fine, but it _also_ removes comments, which - I think - is > quite a bit wrong. 1-3 are also candidates for going through my qapi tree (I will probably do another pull request Monday or Tuesday), if that beats your trivial tree. But if not, for 1-3, you can add: Acked-by: Eric Blake <eblake@redhat.com> > > I can apply just selected hunks, but.. hmm.. v2 of patch 4 is better, and the trivial tree may indeed be a better place for patch 4 since it doesn't really touch anything qapi-related.