| Message ID | 20171215181810.4122-1-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | checkpatch: warn when using volatile with a comment | expand |
Hi,
This series seems to have some coding style problems. See output below for
more information:
Type: series
Message-id: 20171215181810.4122-1-marcandre.lureau@redhat.com
Subject: [Qemu-devel] [PATCH] checkpatch: warn when using volatile with a comment
=== 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
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/20171215150659.1811-1-marcandre.lureau@redhat.com -> patchew/20171215150659.1811-1-marcandre.lureau@redhat.com
t [tag update] patchew/20171215181810.4122-1-marcandre.lureau@redhat.com -> patchew/20171215181810.4122-1-marcandre.lureau@redhat.com
Switched to a new branch 'test'
80828ed1b5 checkpatch: warn when using volatile with a comment
=== OUTPUT BEGIN ===
Checking PATCH 1/1: checkpatch: warn when using volatile with a comment...
ERROR: line over 90 characters
#28: FILE: scripts/checkpatch.pl:2479:
+ my $msg = "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr;
total: 1 errors, 0 warnings, 13 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 15/12/2017 19:18, Marc-André Lureau wrote: > Instead of an error, lower to a warning message, assuming the comment > gives some justification. > > Discussed in: > '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process to debug"' > > Suggested-by: Fam Zheng <famz@redhat.com> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> We can drop the error at all if there is a comment. Also, "volatile sig_atomic_t" is probably self-explanatory and usually correct. So what about this: diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index f5a523af10..3dc27d9656 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2475,13 +2475,11 @@ sub process { # no volatiles please my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; - if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) { - my $msg = "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr; - if (ctx_has_comment($first_line, $linenr)) { - WARN($msg); - } else { - ERROR($msg); - } + if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ && + $line !~ /sig_atomic_t/ && + !ctx_has_comment($first_line, $linenr)) { + my $msg = "Use of volatile is usually wrong, please add a comment\n" . $herecurr; + ERROR($msg); } # warn about #if 0
Hi On Mon, Dec 18, 2017 at 1:49 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: > On 15/12/2017 19:18, Marc-André Lureau wrote: >> Instead of an error, lower to a warning message, assuming the comment >> gives some justification. >> >> Discussed in: >> '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process to debug"' >> >> Suggested-by: Fam Zheng <famz@redhat.com> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > We can drop the error at all if there is a comment. Also, "volatile sig_atomic_t" is probably self-explanatory and usually correct. So what about this: > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index f5a523af10..3dc27d9656 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2475,13 +2475,11 @@ sub process { > > # no volatiles please > my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; > - if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) { > - my $msg = "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr; > - if (ctx_has_comment($first_line, $linenr)) { > - WARN($msg); > - } else { > - ERROR($msg); > - } > + if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ && > + $line !~ /sig_atomic_t/ && > + !ctx_has_comment($first_line, $linenr)) { > + my $msg = "Use of volatile is usually wrong, please add a comment\n" . $herecurr; > + ERROR($msg); > } > Fine for me, can you send a proper patch? Thanks
On 18/12/2017 13:54, Marc-André Lureau wrote: > Hi > > On Mon, Dec 18, 2017 at 1:49 PM, Paolo Bonzini <pbonzini@redhat.com> wrote: >> On 15/12/2017 19:18, Marc-André Lureau wrote: >>> Instead of an error, lower to a warning message, assuming the comment >>> gives some justification. >>> >>> Discussed in: >>> '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process to debug"' >>> >>> Suggested-by: Fam Zheng <famz@redhat.com> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> We can drop the error at all if there is a comment. Also, "volatile sig_atomic_t" is probably self-explanatory and usually correct. So what about this: >> >> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >> index f5a523af10..3dc27d9656 100755 >> --- a/scripts/checkpatch.pl >> +++ b/scripts/checkpatch.pl >> @@ -2475,13 +2475,11 @@ sub process { >> >> # no volatiles please >> my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; >> - if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) { >> - my $msg = "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr; >> - if (ctx_has_comment($first_line, $linenr)) { >> - WARN($msg); >> - } else { >> - ERROR($msg); >> - } >> + if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ && >> + $line !~ /sig_atomic_t/ && >> + !ctx_has_comment($first_line, $linenr)) { >> + my $msg = "Use of volatile is usually wrong, please add a comment\n" . $herecurr; >> + ERROR($msg); >> } >> > > Fine for me, can you send a proper patch? Yes, will do. Paolo
Hi Paolo, Slight nit on the subject line, did you mean to s/with/without/ - that seems to reflect the change in the patch more correctly. Thanks, Darren. On Mon, Dec 18, 2017 at 01:49:52PM +0100, Paolo Bonzini wrote: >On 15/12/2017 19:18, Marc-André Lureau wrote: >> Instead of an error, lower to a warning message, assuming the comment >> gives some justification. >> >> Discussed in: >> '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process to debug"' >> >> Suggested-by: Fam Zheng <famz@redhat.com> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > >We can drop the error at all if there is a comment. Also, "volatile sig_atomic_t" is probably self-explanatory and usually correct. So what about this: > >diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >index f5a523af10..3dc27d9656 100755 >--- a/scripts/checkpatch.pl >+++ b/scripts/checkpatch.pl >@@ -2475,13 +2475,11 @@ sub process { > > # no volatiles please > my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; >- if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) { >- my $msg = "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr; >- if (ctx_has_comment($first_line, $linenr)) { >- WARN($msg); >- } else { >- ERROR($msg); >- } >+ if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ && >+ $line !~ /sig_atomic_t/ && >+ !ctx_has_comment($first_line, $linenr)) { >+ my $msg = "Use of volatile is usually wrong, please add a comment\n" . $herecurr; >+ ERROR($msg); > } > > # warn about #if 0 >
Nevermind, saw that updated comment in the later patch... Thanks, Darren. On Mon, Dec 18, 2017 at 01:36:52PM +0000, Darren Kenny wrote: >Hi Paolo, > >Slight nit on the subject line, did you mean to s/with/without/ - >that seems to reflect the change in the patch more correctly. > >Thanks, > >Darren. > >On Mon, Dec 18, 2017 at 01:49:52PM +0100, Paolo Bonzini wrote: >>On 15/12/2017 19:18, Marc-André Lureau wrote: >>>Instead of an error, lower to a warning message, assuming the comment >>>gives some justification. >>> >>>Discussed in: >>>'[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process to debug"' >>> >>>Suggested-by: Fam Zheng <famz@redhat.com> >>>Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> >>We can drop the error at all if there is a comment. Also, "volatile sig_atomic_t" is probably self-explanatory and usually correct. So what about this: >> >>diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl >>index f5a523af10..3dc27d9656 100755 >>--- a/scripts/checkpatch.pl >>+++ b/scripts/checkpatch.pl >>@@ -2475,13 +2475,11 @@ sub process { >> >># no volatiles please >> my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; >>- if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) { >>- my $msg = "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr; >>- if (ctx_has_comment($first_line, $linenr)) { >>- WARN($msg); >>- } else { >>- ERROR($msg); >>- } >>+ if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/ && >>+ $line !~ /sig_atomic_t/ && >>+ !ctx_has_comment($first_line, $linenr)) { >>+ my $msg = "Use of volatile is usually wrong, please add a comment\n" . $herecurr; >>+ ERROR($msg); >> } >> >># warn about #if 0 >>
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 34df753571..f5a523af10 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2476,7 +2476,12 @@ sub process { # no volatiles please my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b}; if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) { - ERROR("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr); + my $msg = "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr; + if (ctx_has_comment($first_line, $linenr)) { + WARN($msg); + } else { + ERROR($msg); + } } # warn about #if 0
Instead of an error, lower to a warning message, assuming the comment gives some justification. Discussed in: '[Qemu-devel] [PATCH] dump-guest-memory.py: fix "You can't do that without a process to debug"' Suggested-by: Fam Zheng <famz@redhat.com> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- scripts/checkpatch.pl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-)