| 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.