2008-10-21 00:57:53

by Tetsuo Handa

[permalink] [raw]
Subject: */ in string confuses checkpatch.pl

Hello.

The below code confuses checkpatch.pl ver 0.21.

Regards.
----------
# cat /tmp/foo.c
void foo(void)
{
bar(\" /proc/\\\\*/\");
bar(\" /proc/\\\\$/\");
}
# /usr/src/vanilla/linux-2.6.27.2/scripts/checkpatch.pl --file /tmp/foo.c
ERROR: need consistent spacing around \'/\' (ctx:WxV)
#4: FILE: tmp/foo.c:4:
+ bar(\" /proc/\\\\$/\");
^

total: 1 errors, 0 warnings, 5 lines checked

/tmp/foo.c has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.


2008-10-22 07:56:22

by Andy Whitcroft

[permalink] [raw]
Subject: Re: */ in string confuses checkpatch.pl

On Tue, Oct 21, 2008 at 09:57:26AM +0900, [email protected] wrote:
> Hello.
>
> The below code confuses checkpatch.pl ver 0.21.
>
> Regards.
> ----------
> # cat /tmp/foo.c
> void foo(void)
> {
> bar(\" /proc/\\\\*/\");
> bar(\" /proc/\\\\$/\");
> }
> # /usr/src/vanilla/linux-2.6.27.2/scripts/checkpatch.pl --file /tmp/foo.c
> ERROR: need consistent spacing around \'/\' (ctx:WxV)
> #4: FILE: tmp/foo.c:4:
> + bar(\" /proc/\\\\$/\");
> ^
>
> total: 1 errors, 0 warnings, 5 lines checked
>
> /tmp/foo.c has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.

I am unable to reproduce this here with any version from current back to
v0.19, nor with the one I find in v2.6.27.2 (see below). Though looking at
the code as presented in your example I can see how it might be interpreted
incorrectly. That said the compiler doesn't seem to be able to understand
this either (also below). In particular you effectivly open a quote on
the third line and never close it.

Could you send me both the checkpatch script and the foo.c as attachments
so I can be sure I have them without some emailer somewhere mushing
them up.

Thanks for you report.

-apw

$ cat ../checkpatch/Z213.c
void foo(void)
{
bar(\" /proc/\\\\*/\");
bar(\" /proc/\\\\$/\");
}
$ cc -c Z213.c
Z213.c: In function ‘foo’:
Z213.c:3: error: stray ‘\’ in program
Z213.c:3:7: warning: missing terminating " character
Z213.c:3: error: missing terminating " character
Z213.c:4: error: stray ‘\’ in program
Z213.c:4:7: warning: missing terminating " character
Z213.c:4: error: missing terminating " character
Z213.c:5: error: expected expression before ‘}’ token
Z213.c:5: error: expected ‘)’ before ‘}’ token
Z213.c:5: error: expected ‘;’ before ‘}’ token

$ git checkout v2.6.27.2
HEAD is now at 6bcd6d7... Linux 2.6.27.2
apw@brain$ ./scripts/checkpatch.pl --file ../checkpatch/Z213.c
total: 0 errors, 0 warnings, 5 lines checked

../checkpatch/Z213.c has no obvious style problems and is ready for submission.
$

2008-10-22 08:03:39

by Tetsuo Handa

[permalink] [raw]
Subject: Re: */ in string confuses checkpatch.pl

Hello.

> I am unable to reproduce this here with any version
I'm sorry. Webmail system broke the source code.

Please see http://lkml.org/lkml/2008/10/21/114 and try to reproduce.

Regards.

2008-10-22 08:31:46

by Andy Whitcroft

[permalink] [raw]
Subject: Re: */ in string confuses checkpatch.pl

On Wed, Oct 22, 2008 at 08:56:17AM +0100, Andy Whitcroft wrote:
> On Tue, Oct 21, 2008 at 09:57:26AM +0900, [email protected] wrote:
> > Hello.
> >
> > The below code confuses checkpatch.pl ver 0.21.
> >
> > Regards.
> > ----------
> > # cat /tmp/foo.c
> > void foo(void)
> > {
> > bar(\" /proc/\\\\*/\");
> > bar(\" /proc/\\\\$/\");
> > }
> > # /usr/src/vanilla/linux-2.6.27.2/scripts/checkpatch.pl --file /tmp/foo.c
> > ERROR: need consistent spacing around \'/\' (ctx:WxV)
> > #4: FILE: tmp/foo.c:4:
> > + bar(\" /proc/\\\\$/\");
> > ^
> >
> > total: 1 errors, 0 warnings, 5 lines checked
> >
> > /tmp/foo.c has style problems, please review. If any of these errors
> > are false positives report them to the maintainer, see
> > CHECKPATCH in MAINTAINERS.

Ok, I can see whats happened here. Most of these \'s are extraneous.
Without those I can reproduce this. Its a bug in the 'comment is open
at the start of hunk' detection. I think I have updated this heuristic
to cope wit this. Could you try your original patch (I assume there was
one) with the version below:

http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing

Thanks for your report.

-apw

2008-10-22 11:21:43

by Tetsuo Handa

[permalink] [raw]
Subject: Re: */ in string confuses checkpatch.pl

Hello.

Andy Whitcroft wrote:
> Ok, I can see whats happened here. Most of these \'s are extraneous.
> Without those I can reproduce this. Its a bug in the 'comment is open
> at the start of hunk' detection. I think I have updated this heuristic
> to cope wit this. Could you try your original patch (I assume there was
> one) with the version below:
>
> http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-testing
>
> Thanks for your report.
>
> -apw
>
It solved this problem.

By the way, I'm using checkpatch.pl for checking userland application.
It would be convenient if there is a option that omits some checks like
'# check for external initialisers.' and '# check for static initialisers.'

Thank you.

2008-10-23 11:27:47

by Tetsuo Handa

[permalink] [raw]
Subject: [checkpatch.pl] two bugs

Hello.

I think these are bugs.

Regards.
-----

Bug 1:
assignment in 'if' is not warned if '\n' is inserted before the assignment.

# cat /tmp/test1.c
void foo(void)
{
int i = 0;
if (0 ||
(i = 0) != 0)
return;
}
# /usr/src/vanilla/linux-2.6.27.3/scripts/checkpatch.pl --file /tmp/test1.c
total: 0 errors, 0 warnings, 7 lines checked

/tmp/test1.c has no obvious style problems and is ready for submission.
# ./scripts/checkpatch.pl-testing --strict --file /tmp/test1.c
total: 0 errors, 0 warnings, 0 checks, 7 lines checked

/tmp/test1.c has no obvious style problems and is ready for submission.



Bug 2:
'while' placed after 'if { }' block is not handled properly.

# cat /tmp/test2.c
int i;
void foo(void)
{
if (!i) {
i++;
return;
}
while (i);
}
# /usr/src/vanilla/linux-2.6.27.3/scripts/checkpatch.pl --file /tmp/test2.c
ERROR: trailing statements should be on next line
#8: FILE: tmp/test2.c:8:
+ while (i);

ERROR: while should follow close brace '}'
#8: FILE: tmp/test2.c:8:
+ }
+ while (i);

total: 2 errors, 0 warnings, 9 lines checked

/tmp/test2.c has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
# ./scripts/checkpatch.pl-testing --strict --file /tmp/test2.c
ERROR: trailing statements should be on next line
#8: FILE: tmp/test2.c:8:
+ while (i);
+ while (i);
ERROR: while should follow close brace '}'
#8: FILE: tmp/test2.c:8:
+ }
+ while (i);

total: 2 errors, 0 warnings, 0 checks, 9 lines checked

/tmp/test2.c has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2008-10-23 11:54:29

by Andy Whitcroft

[permalink] [raw]
Subject: Re: [checkpatch.pl] two bugs

On Thu, Oct 23, 2008 at 08:27:34PM +0900, Tetsuo Handa wrote:
> Hello.
>
> I think these are bugs.
>
> Regards.
> -----
>
> Bug 1:
> assignment in 'if' is not warned if '\n' is inserted before the assignment.

Yep that looks wrong, too line oriented.

> # cat /tmp/test1.c
> void foo(void)
> {
> int i = 0;
> if (0 ||
> (i = 0) != 0)
> return;
> }
> # /usr/src/vanilla/linux-2.6.27.3/scripts/checkpatch.pl --file /tmp/test1.c
> total: 0 errors, 0 warnings, 7 lines checked
>
> /tmp/test1.c has no obvious style problems and is ready for submission.
> # ./scripts/checkpatch.pl-testing --strict --file /tmp/test1.c
> total: 0 errors, 0 warnings, 0 checks, 7 lines checked
>
> /tmp/test1.c has no obvious style problems and is ready for submission.
>
>
>
> Bug 2:
> 'while' placed after 'if { }' block is not handled properly.
>
> # cat /tmp/test2.c
> int i;
> void foo(void)
> {
> if (!i) {
> i++;
> return;
> }
> while (i);
> }
> # /usr/src/vanilla/linux-2.6.27.3/scripts/checkpatch.pl --file /tmp/test2.c
> ERROR: trailing statements should be on next line
> #8: FILE: tmp/test2.c:8:
> + while (i);
>
> ERROR: while should follow close brace '}'
> #8: FILE: tmp/test2.c:8:
> + }
> + while (i);
>
> total: 2 errors, 0 warnings, 9 lines checked
>
> /tmp/test2.c has style problems, please review. If any of these errors
> are false positives report them to the maintainer, see
> CHECKPATCH in MAINTAINERS.
> # ./scripts/checkpatch.pl-testing --strict --file /tmp/test2.c
> ERROR: trailing statements should be on next line
> #8: FILE: tmp/test2.c:8:
> + while (i);
> + while (i);

Hmmm, we have done a lot of work here to figure out this is a while
without a do and therefore the ; should be on the next line; yet we then
let the looser check below fire too. A bug indeed.

> ERROR: while should follow close brace '}'
> #8: FILE: tmp/test2.c:8:
> + }
> + while (i);
>
> total: 2 errors, 0 warnings, 0 checks, 9 lines checked

Thanks for your reports. Will get them on the tofix list.

-apw