[Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Peter Maydell]

In checkpatch we attempt to check for and warn about
block comments which start with /* or /** followed by a
non-blank. Unfortunately a bug in the regex meant that
we would incorrectly warn about comments starting with
"/**" with no following text:

  git show 9813dc6ac3954d58ba16b3920556f106f97e1c67|./scripts/checkpatch.pl -
  WARNING: Block comments use a leading /* on a separate line
  #34: FILE: tests/libqtest.h:233:
  +/**

The sequence "/\*\*?" was intended to match either "/*" or "/**",
but Perl's semantics for '?' allow it to backtrack and try the
"matches 0 chars" option if the "matches 1 char" choice leads to
a failure of the rest of the regex to match.  Switch to "/\*\*?+"
which uses what perlre(1) calls the "possessive" quantifier form:
this means that if it matches the "/**" string it will not later
backtrack to matching just the "/*" prefix.

Reported-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
This comment check is unique to QEMU checkpatch so the bug
doesn't exist in the Linux version.
---
 scripts/checkpatch.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d10dddf1be4..5f1ec537d21 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1624,7 +1624,7 @@ sub process {
 
 		# Block comments use /* on a line of its own
 		if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&	#inline /*...*/
-		    $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank
+		    $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** non-blank
 			WARN("Block comments use a leading /* on a separate line\n" . $herecurr);
 		}
 
-- 
2.20.1

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2420 bytes --]

On 1/18/19 7:27 AM, Peter Maydell wrote:
> In checkpatch we attempt to check for and warn about
> block comments which start with /* or /** followed by a
> non-blank. Unfortunately a bug in the regex meant that
> we would incorrectly warn about comments starting with
> "/**" with no following text:
> 
>   git show 9813dc6ac3954d58ba16b3920556f106f97e1c67|./scripts/checkpatch.pl -
>   WARNING: Block comments use a leading /* on a separate line
>   #34: FILE: tests/libqtest.h:233:
>   +/**
> 
> The sequence "/\*\*?" was intended to match either "/*" or "/**",
> but Perl's semantics for '?' allow it to backtrack and try the
> "matches 0 chars" option if the "matches 1 char" choice leads to
> a failure of the rest of the regex to match.  Switch to "/\*\*?+"
> which uses what perlre(1) calls the "possessive" quantifier form:
> this means that if it matches the "/**" string it will not later
> backtrack to matching just the "/*" prefix.

Just wondering if "/\*{1,2}" would also work (it may have to be spelled
"/\*\{1,2}" - I never remember which flavors of regex have which
extensions without rereading docs).  But since your way is tested, I'm
not going to force a respin.

> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> This comment check is unique to QEMU checkpatch so the bug
> doesn't exist in the Linux version.
> ---
>  scripts/checkpatch.pl | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d10dddf1be4..5f1ec537d21 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1624,7 +1624,7 @@ sub process {
>  
>  		# Block comments use /* on a line of its own
>  		if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&	#inline /*...*/
> -		    $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank
> +		    $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** non-blank

Hmm - Isn't "[ \t]*.+[ \t]*$" the same as ".+$?"  Perhaps:

m@^\+.*/\*([^*]|\*.)@

also does the trick in a more legible way (that is, any line that starts
with /* and then contains any additional characters other than a second *)?

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Peter Maydell]

On Fri, 18 Jan 2019 at 15:53, Eric Blake <eblake@redhat.com> wrote:
>
> On 1/18/19 7:27 AM, Peter Maydell wrote:
> > In checkpatch we attempt to check for and warn about
> > block comments which start with /* or /** followed by a
> > non-blank. Unfortunately a bug in the regex meant that
> > we would incorrectly warn about comments starting with
> > "/**" with no following text:
> >
> >   git show 9813dc6ac3954d58ba16b3920556f106f97e1c67|./scripts/checkpatch.pl -
> >   WARNING: Block comments use a leading /* on a separate line
> >   #34: FILE: tests/libqtest.h:233:
> >   +/**
> >
> > The sequence "/\*\*?" was intended to match either "/*" or "/**",
> > but Perl's semantics for '?' allow it to backtrack and try the
> > "matches 0 chars" option if the "matches 1 char" choice leads to
> > a failure of the rest of the regex to match.  Switch to "/\*\*?+"
> > which uses what perlre(1) calls the "possessive" quantifier form:
> > this means that if it matches the "/**" string it will not later
> > backtrack to matching just the "/*" prefix.
>
> Just wondering if "/\*{1,2}" would also work (it may have to be spelled
> "/\*\{1,2}" - I never remember which flavors of regex have which
> extensions without rereading docs)

Oh yes, that would probably be the less perl-specific way
to write it.

Perl regexes have the convenient property that backslash
followed by a punctuation character always means "that
character literally", whether or not that punctuation
character has a metacharacter meaning when not escaped.
So it's definitely the non-backslash version.
(As an aside, it's a bit sad that Rust regexes don't have
this property.)

> > This comment check is unique to QEMU checkpatch so the bug
> > doesn't exist in the Linux version.
> > ---
> >  scripts/checkpatch.pl | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index d10dddf1be4..5f1ec537d21 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -1624,7 +1624,7 @@ sub process {
> >
> >               # Block comments use /* on a line of its own
> >               if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&      #inline /*...*/
> > -                 $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank
> > +                 $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** non-blank
>
> Hmm - Isn't "[ \t]*.+[ \t]*$" the same as ".+$?"

(do you mean '$?"' at the end of your sentence, or '$" ?' ?)

I'm not sure exactly what I was aiming for with that
part of the regex when I wrote it. The comment suggests
that I was looking for "non-blank", ie I didn't want to
fire on /* or /** followed by just trailing whitespace.
The regex as written is clearly not actually doing that,
though...

-- PMM

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 2423 bytes --]

On 1/18/19 10:08 AM, Peter Maydell wrote:

>>> The sequence "/\*\*?" was intended to match either "/*" or "/**",
>>> but Perl's semantics for '?' allow it to backtrack and try the
>>> "matches 0 chars" option if the "matches 1 char" choice leads to
>>> a failure of the rest of the regex to match.  Switch to "/\*\*?+"
>>> which uses what perlre(1) calls the "possessive" quantifier form:
>>> this means that if it matches the "/**" string it will not later
>>> backtrack to matching just the "/*" prefix.
>>
>> Just wondering if "/\*{1,2}" would also work (it may have to be spelled
>> "/\*\{1,2}" - I never remember which flavors of regex have which
>> extensions without rereading docs)
> 
> Oh yes, that would probably be the less perl-specific way
> to write it.

Except it would probably fail in the same way as the non-greedy \*? -
when \*{2} can't satisfy the regex, the engine will retry with \*{1} and
then see the second * as noise.


>>>               # Block comments use /* on a line of its own
>>>               if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&      #inline /*...*/
>>> -                 $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank
>>> +                 $rawline =~ m@^\+.*/\*\*?+[ \t]*.+[ \t]*$@) { # /* or /** non-blank
>>
>> Hmm - Isn't "[ \t]*.+[ \t]*$" the same as ".+$?"
> 
> (do you mean '$?"' at the end of your sentence, or '$" ?' ?)

Serves me right for adding "" after the fact.  You are correct, intended
punctuation should be ".+$"? (the 3-character regex, concluded by asking
a question if the 3-character form is the same as the 15-character
form), and where I should have gone further and asked if it was the same
as "." (since who cares whether there are more than one extra characters
between there and the end of the line - once we've found any character
at all, we've already found enough to flag the line).

> 
> I'm not sure exactly what I was aiming for with that
> part of the regex when I wrote it. The comment suggests
> that I was looking for "non-blank", ie I didn't want to
> fire on /* or /** followed by just trailing whitespace.
> The regex as written is clearly not actually doing that,
> though...

Trailing whitespace is generally a problem anyways, though :)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH] checkpatch: Don't spuriously warn about /** comment starters

[Peter Maydell]

On Fri, 18 Jan 2019 at 16:26, Eric Blake <eblake@redhat.com> wrote:
>
> On 1/18/19 10:08 AM, Peter Maydell wrote:
>
> >>> The sequence "/\*\*?" was intended to match either "/*" or "/**",
> >>> but Perl's semantics for '?' allow it to backtrack and try the
> >>> "matches 0 chars" option if the "matches 1 char" choice leads to
> >>> a failure of the rest of the regex to match.  Switch to "/\*\*?+"
> >>> which uses what perlre(1) calls the "possessive" quantifier form:
> >>> this means that if it matches the "/**" string it will not later
> >>> backtrack to matching just the "/*" prefix.
> >>
> >> Just wondering if "/\*{1,2}" would also work (it may have to be spelled
> >> "/\*\{1,2}" - I never remember which flavors of regex have which
> >> extensions without rereading docs)
> >
> > Oh yes, that would probably be the less perl-specific way
> > to write it.
>
> Except it would probably fail in the same way as the non-greedy \*? -
> when \*{2} can't satisfy the regex, the engine will retry with \*{1} and
> then see the second * as noise.

OK, so the patch is good as it is, though it doesn't address
the possible incorrect warnings with trailing whitespace.

> > I'm not sure exactly what I was aiming for with that
> > part of the regex when I wrote it. The comment suggests
> > that I was looking for "non-blank", ie I didn't want to
> > fire on /* or /** followed by just trailing whitespace.
> > The regex as written is clearly not actually doing that,
> > though...
>
> Trailing whitespace is generally a problem anyways, though :)

Yes, but checkpatch already checks for that, so we don't want
to report an incorrect "wrong blockquote" warning as well as
the "trailing whitespace" error, which is what the current
code does:

ERROR: trailing whitespace
#159: FILE: hw/arm/mps2-tz.c:391:
+    /* $

WARNING: Block comments use a leading /* on a separate line
#159: FILE: hw/arm/mps2-tz.c:391:
+    /*

total: 1 errors, 1 warnings, 165 lines checked

thanks
-- PMM