Re: [Qemu-trivial] [Qemu-devel] [PATCH RFC v3 for-2.12?] scripts/checkpatch.pl: Bug fix

[Su Hang]

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" <address@hidden>
> Sent Time: 2018-04-05 02:28:13 (Thursday)
> To: "Su Hang" <address@hidden>, address@hidden
> Cc: qemu-trivial <address@hidden>, "Paolo Bonzini" <address@hidden>, 
> address@hidden
> 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 <address@hidden>
> > ---
> > 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 <address@hidden>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3266
> Virtualization:  qemu.org | libvirt.org
>