| Message ID | 1522029982-4650-1-git-send-email-suhang16@mails.ucas.ac.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
[adding a few more cc's] On 03/25/2018 09:06 PM, Su Hang wrote: > Commit 2b9aef6fcd96ba7ed8c1ee723e391901852d344c introduced a regression: > checkpatch.pl started complaining about the following valid pattern: > do { > /* something */ > } while (condition); > > Fix the script to once again permit this pattern. We can probably drop the RFC from the title (RFC means you are unsure if the patch is in its final form), and probably want this patch included in 2.12 as we are still getting emails that hit the false positive: https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00403.html > > Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn> > --- > v1: fix bug. > v2: correct inappropriate patch description. > v3: put version description under Signed-off-by line. > > scripts/checkpatch.pl | 15 +++++++++++++-- > 1 file changed, 13 insertions(+), 2 deletions(-) Perl is not my strongest point, so take my review with a grain of salt. However, since I already have a couple of other random patches that may still be appropriate for -rc3, I can pick this up in a pull request if I get at least one more review (and if no one else picks it up first, such as the qemu-trivial process or Paolo's misc tree) > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 57daae05ea18..d52207a3cc9d 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -2356,6 +2356,18 @@ sub process { > # check for missing bracing around if etc > if ($line =~ /(^.*)\b(?:if|while|for)\b/ && > $line !~ /\#\s*if/) { > + my $allowed = 0; > + > + # Check the pre-context. > + if ($line =~ /(\}.*?)$/) { Why are you using minimal match coupled with an anchored expression? Isn't '($line =~ /(\}.*)$/)' going to match the same subexpression (namely, any line containing } but not as the last character)? Otherwise, Reviewed-by: Eric Blake <eblake@redhat.com>
On 04/04/2018 20:28, Eric Blake wrote: > Perl is not my strongest point, so take my review with a grain of salt. > However, since I already have a couple of other random patches that may > still be appropriate for -rc3, I can pick this up in a pull request if I > get at least one more review (and if no one else picks it up first, such > as the qemu-trivial process or Paolo's misc tree) > I'll pick this up. Thanks! Paolo
Sorry for replying late, it's until today that I saw your mail.
In order to find out why the former edition doesn't complain about
`do{}while(cond);` pattern, I regress back to
ed279a06c53784c8c6c9b41aa0388a4ce8a70410, one before the bug was introduced.
Then I found in Line 2435 to Line 2443 did special judgment for
`do{}while(cond);` pattern.
As for why I use `if ($line !~ /else/)` instead of `if ($line =~ /while/)`,
And why I use `($line =~ /(\}.*)$/)`, instead of `(substr($line, 0, $-[0]) =~ /(\}\s*)$/)`.
Since they work the same, so I'm trying to minimize the modification
to current code and not to introduce new code logic, I reuse most of
2435 - 2443 Lines from ed279a06c53784c8c6c9b41aa0388a4ce8a70410 in my patch.
> Why are you using minimal match coupled with an anchored expression?
> Isn't '($line =~ /(\}.*)$/)' going to match the same subexpression
> (namely, any line containing } but not as the last character)?
'($line =~ /(\}.*)$/)' won't match "any line containing } but not as
the last character", becuase
```
if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
$line !~ /\#\s*if/) {
``` that wraps '($line =~ /(\}.*)$/)' limits the scope of the match.
Best,
Su Hang
> -----Original Messages-----
> From: "Eric Blake" <eblake@redhat.com>
> Sent Time: 2018-04-05 02:28:13 (Thursday)
> To: "Su Hang" <suhang16@mails.ucas.ac.cn>, vsementsov@virtuozzo.com
> Cc: qemu-trivial <qemu-trivial@nongnu.org>, "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH RFC v3 for-2.12?] scripts/checkpatch.pl: Bug fix
>
> [adding a few more cc's]
>
> On 03/25/2018 09:06 PM, Su Hang wrote:
> > Commit 2b9aef6fcd96ba7ed8c1ee723e391901852d344c introduced a regression:
> > checkpatch.pl started complaining about the following valid pattern:
> > do {
> > /* something */
> > } while (condition);
> >
> > Fix the script to once again permit this pattern.
>
> We can probably drop the RFC from the title (RFC means you are unsure if
> the patch is in its final form), and probably want this patch included
> in 2.12 as we are still getting emails that hit the false positive:
>
> https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg00403.html
>
> >
> > Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn>
> > ---
> > v1: fix bug.
> > v2: correct inappropriate patch description.
> > v3: put version description under Signed-off-by line.
> >
> > scripts/checkpatch.pl | 15 +++++++++++++--
> > 1 file changed, 13 insertions(+), 2 deletions(-)
>
> Perl is not my strongest point, so take my review with a grain of salt.
> However, since I already have a couple of other random patches that may
> still be appropriate for -rc3, I can pick this up in a pull request if I
> get at least one more review (and if no one else picks it up first, such
> as the qemu-trivial process or Paolo's misc tree)
>
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 57daae05ea18..d52207a3cc9d 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -2356,6 +2356,18 @@ sub process {
> > # check for missing bracing around if etc
> > if ($line =~ /(^.*)\b(?:if|while|for)\b/ &&
> > $line !~ /\#\s*if/) {
> > + my $allowed = 0;
> > +
> > + # Check the pre-context.
> > + if ($line =~ /(\}.*?)$/) {
>
> Why are you using minimal match coupled with an anchored expression?
> Isn't '($line =~ /(\}.*)$/)' going to match the same subexpression
> (namely, any line containing } but not as the last character)?
>
> Otherwise,
> Reviewed-by: Eric Blake <eblake@redhat.com>
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3266
> Virtualization: qemu.org | libvirt.org
>
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 57daae05ea18..d52207a3cc9d 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2356,6 +2356,18 @@ sub process { # check for missing bracing around if etc if ($line =~ /(^.*)\b(?:if|while|for)\b/ && $line !~ /\#\s*if/) { + my $allowed = 0; + + # Check the pre-context. + if ($line =~ /(\}.*?)$/) { + my $pre = $1; + + if ($line !~ /else/) { + print "APW: ALLOWED: pre<$pre> line<$line>\n" + if $dbg_adv_apw; + $allowed = 1; + } + } my ($level, $endln, @chunks) = ctx_statement_full($linenr, $realcnt, 1); if ($dbg_adv_apw) { @@ -2364,7 +2376,6 @@ sub process { if $#chunks >= 1; } if ($#chunks >= 0 && $level == 0) { - my $allowed = 0; my $seen = 0; my $herectx = $here . "\n"; my $ln = $linenr - 1; @@ -2408,7 +2419,7 @@ sub process { $allowed = 1; } } - if ($seen != ($#chunks + 1)) { + if ($seen != ($#chunks + 1) && !$allowed) { ERROR("braces {} are necessary for all arms of this statement\n" . $herectx); } }
Commit 2b9aef6fcd96ba7ed8c1ee723e391901852d344c introduced a regression: checkpatch.pl started complaining about the following valid pattern: do { /* something */ } while (condition); Fix the script to once again permit this pattern. Signed-off-by: Su Hang <suhang16@mails.ucas.ac.cn> --- v1: fix bug. v2: correct inappropriate patch description. v3: put version description under Signed-off-by line. scripts/checkpatch.pl | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-)