bug in checkpatch (on pointers to typedefs?)

bug in checkpatch (on pointers to typedefs?)

[Marcin Slusarz]

Hi

Checkpatch in current mainline outputs following errors:

$ ./scripts/checkpatch.pl -q --file ./fs/udf/misc.c
ERROR: need consistent spacing around '*' (ctx:WxV)
#205: FILE: fs/udf/misc.c:205:
+       tag *tag_p;
            ^

$ ./scripts/checkpatch.pl -q --file ./fs/udf/unicode.c
ERROR: need consistent spacing around '*' (ctx:WxV)
#48: FILE: fs/udf/unicode.c:48:
+int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
                                               ^
(...)

I think the code is ok.

Marcin

Re: bug in checkpatch (on pointers to typedefs?)

[Andy Whitcroft]

On Sun, Feb 10, 2008 at 03:33:02PM +0100, Marcin Slusarz wrote:
> Hi
> 
> Checkpatch in current mainline outputs following errors:
> 
> $ ./scripts/checkpatch.pl -q --file ./fs/udf/misc.c
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #205: FILE: fs/udf/misc.c:205:
> +       tag *tag_p;
>             ^
> 
> $ ./scripts/checkpatch.pl -q --file ./fs/udf/unicode.c
> ERROR: need consistent spacing around '*' (ctx:WxV)
> #48: FILE: fs/udf/unicode.c:48:
> +int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
>                                                ^
> (...)
> 
> I think the code is ok.

Yep the code is clearly correct.  Can I have the whole patch fragment
you got these against so I can figure out why we are unable to detect
these two as types, I would expect us to have done so.  Also what
version of checkpatch is this?  There is a version string at the top.

-apw

Re: bug in checkpatch (on pointers to typedefs?)

[Benny Halevy]

I saw this too with checkpatch.pl version 0.12
It seems like checkpatch.pl knows only about types derived
from @typeList by build_types.

Example below...

Benny

$ cat <<EOF | scripts/checkpatch.pl -
Signed-off-by: john@smith.net
---
diff a/f.c b/f.c
--- a/f.c
+++ b/f.c
@@ -1,0 +1,2 @@
+foo(int a, my_uint32 *);
+bar(int a, my_uint32_t *);
EOF
ERROR: need consistent spacing around '*' (ctx:WxB)
#7: FILE: f.c:1:
+foo(int a, my_uint32 *);
                      ^

total: 1 errors, 0 warnings, 2 lines checked

On Feb. 11, 2008, 12:23 +0200, Andy Whitcroft <apw@shadowen.org> wrote:
> On Sun, Feb 10, 2008 at 03:33:02PM +0100, Marcin Slusarz wrote:
>> Hi
>>
>> Checkpatch in current mainline outputs following errors:
>>
>> $ ./scripts/checkpatch.pl -q --file ./fs/udf/misc.c
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> #205: FILE: fs/udf/misc.c:205:
>> +       tag *tag_p;
>>             ^
>>
>> $ ./scripts/checkpatch.pl -q --file ./fs/udf/unicode.c
>> ERROR: need consistent spacing around '*' (ctx:WxV)
>> #48: FILE: fs/udf/unicode.c:48:
>> +int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
>>                                                ^
>> (...)
>>
>> I think the code is ok.
> 
> Yep the code is clearly correct.  Can I have the whole patch fragment
> you got these against so I can figure out why we are unable to detect
> these two as types, I would expect us to have done so.  Also what
> version of checkpatch is this?  There is a version string at the top.
> 
> -apw
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: bug in checkpatch (on pointers to typedefs?)

[Andy Whitcroft]

On Mon, Feb 11, 2008 at 06:05:48PM +0200, Benny Halevy wrote:
> I saw this too with checkpatch.pl version 0.12
> It seems like checkpatch.pl knows only about types derived
> from @typeList by build_types.
> 
> Example below...
> 
> Benny
> 
> $ cat <<EOF | scripts/checkpatch.pl -
> Signed-off-by: john@smith.net
> ---
> diff a/f.c b/f.c
> --- a/f.c
> +++ b/f.c
> @@ -1,0 +1,2 @@
> +foo(int a, my_uint32 *);
> +bar(int a, my_uint32_t *);

But that isn't actually syntactically correct code is it?  You have types
as parameters like a function declaration, but no return type.  So there
is no hint to checkpatch that this is a function declaration and therefore
the parameters are not expected to be types, nor are they checked as such.

The following diff is clean on the latest version of checkpatch:

Signed-off-by: john@smith.net
---
diff a/f.c b/f.c
--- a/f.c
+++ b/f.c
@@ -1,0 +1,2 @@
+void foo(int a, my_uint32 *);
+int bar(int a, my_uint32_t *);
EOF

Could you try out the version of checkpatch at the URL below on the real
patch you are using to test, and let me know if it works.  There are
a number of improvements to type tracking in the face of ifdef's and
the like.  If it doesn't could I have the hunk which fails:

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

-apw

Re: bug in checkpatch (on pointers to typedefs?)

[Benny Halevy]

On Feb. 11, 2008, 18:40 +0200, Andy Whitcroft <apw@shadowen.org> wrote:
> On Mon, Feb 11, 2008 at 06:05:48PM +0200, Benny Halevy wrote:
>> I saw this too with checkpatch.pl version 0.12
>> It seems like checkpatch.pl knows only about types derived
>> from @typeList by build_types.
>>
>> Example below...
>>
>> Benny
>>
>> $ cat <<EOF | scripts/checkpatch.pl -
>> Signed-off-by: john@smith.net
>> ---
>> diff a/f.c b/f.c
>> --- a/f.c
>> +++ b/f.c
>> @@ -1,0 +1,2 @@
>> +foo(int a, my_uint32 *);
>> +bar(int a, my_uint32_t *);
> 
> But that isn't actually syntactically correct code is it?  You have types
> as parameters like a function declaration, but no return type.  So there
> is no hint to checkpatch that this is a function declaration and therefore
> the parameters are not expected to be types, nor are they checked as such.
> 
> The following diff is clean on the latest version of checkpatch:
> 
> Signed-off-by: john@smith.net
> ---
> diff a/f.c b/f.c
> --- a/f.c
> +++ b/f.c
> @@ -1,0 +1,2 @@
> +void foo(int a, my_uint32 *);
> +int bar(int a, my_uint32_t *);
> EOF

OK, but the return type doesn't have to be in the patched line, it could be in
a synchronization line or even missing if the function has a long multi-line argument
list.

> 
> Could you try out the version of checkpatch at the URL below on the real
> patch you are using to test, and let me know if it works.  There are
> a number of improvements to type tracking in the face of ifdef's and
> the like.  If it doesn't could I have the hunk which fails:
> 
>   http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-next

Your modified patch passes with version 0.12 as well as checkpatch.pl-next

However, the following patch that has the return type in the synchronization lines
still produces the same error:

$ ./checkpatch.pl-next -
Signed-off-by: john@smith.net
---
diff a/f.c b/f.c
--- a/f.c
+++ b/f.c
@@ -1,2 +1,4 @@
 int
+foo(int a, my_uint32 *);
 int
+bar(int a, my_uint32_t *);
ERROR: need consistent spacing around '*' (ctx:WxB)
#8: FILE: f.c:2:
+foo(int a, my_uint32 *);
                      ^

total: 1 errors, 0 warnings, 4 lines checked

> 
> -apw
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: bug in checkpatch (on pointers to typedefs?)

[Andy Whitcroft]

On Mon, Feb 11, 2008 at 06:58:08PM +0200, Benny Halevy wrote:
> OK, but the return type doesn't have to be in the patched line, it could be in
> a synchronization line or even missing if the function has a long multi-line argument
> list.

Ok, I guess thats fair criticism.  Could you check out the current
checkpatch-next (0.14-8-g3737366 or later -9, -10 etc), and see if
that works.  It seems to on the simple examples you sent me :).

Thanks.

-apw

Re: bug in checkpatch (on pointers to typedefs?)

[Benny Halevy]

On Feb. 11, 2008, 20:42 +0200, Andy Whitcroft <apw@shadowen.org> wrote:
> On Mon, Feb 11, 2008 at 06:58:08PM +0200, Benny Halevy wrote:
>> OK, but the return type doesn't have to be in the patched line, it could be in
>> a synchronization line or even missing if the function has a long multi-line argument
>> list.
> 
> Ok, I guess thats fair criticism.  Could you check out the current
> checkpatch-next (0.14-8-g3737366 or later -9, -10 etc), and see if
> that works.  It seems to on the simple examples you sent me :).

Confirmed with 0.14-8-g3737366.

Thanks!

Benny

Oh, and I really liked the fact that you print the patch file name
in the summary line of each patch checked rather than "Your patch" :)

> 
> Thanks.
> 
> -apw

Re: bug in checkpatch (on pointers to typedefs?)

[Marcin Slusarz]

On Mon, Feb 11, 2008 at 10:23:39AM +0000, Andy Whitcroft wrote:
> On Sun, Feb 10, 2008 at 03:33:02PM +0100, Marcin Slusarz wrote:
> > Hi
> > 
> > Checkpatch in current mainline outputs following errors:
> > 
> > $ ./scripts/checkpatch.pl -q --file ./fs/udf/misc.c
> > ERROR: need consistent spacing around '*' (ctx:WxV)
> > #205: FILE: fs/udf/misc.c:205:
> > +       tag *tag_p;
> >             ^
> > 
> > $ ./scripts/checkpatch.pl -q --file ./fs/udf/unicode.c
> > ERROR: need consistent spacing around '*' (ctx:WxV)
> > #48: FILE: fs/udf/unicode.c:48:
> > +int udf_build_ustr(struct ustr *dest, dstring *ptr, int size)
> >                                                ^
> > (...)
> > 
> > I think the code is ok.
> 
> Yep the code is clearly correct.  Can I have the whole patch fragment
> you got these against so I can figure out why we are unable to detect
> these two as types, I would expect us to have done so.  Also what
> version of checkpatch is this?  There is a version string at the top.
It's current Linus git tree (both checkpatch and "testcase").

Marcin

Re: bug in checkpatch (on pointers to typedefs?)

[Jan Engelhardt]

On Feb 10 2008 15:33, Marcin Slusarz wrote:
>Checkpatch in current mainline outputs following errors:
>
>$ ./scripts/checkpatch.pl -q --file ./fs/udf/misc.c
>ERROR: need consistent spacing around '*' (ctx:WxV)
>#205: FILE: fs/udf/misc.c:205:
>+       tag *tag_p;
>            ^

I'd say "don't add any new typedefs" (and perhaps get rid of old ones).

Re: bug in checkpatch (on pointers to typedefs?)

[Andy Whitcroft]

On Wed, Feb 13, 2008 at 08:43:58PM +0100, Jan Engelhardt wrote:
> 
> On Feb 10 2008 15:33, Marcin Slusarz wrote:
> >Checkpatch in current mainline outputs following errors:
> >
> >$ ./scripts/checkpatch.pl -q --file ./fs/udf/misc.c
> >ERROR: need consistent spacing around '*' (ctx:WxV)
> >#205: FILE: fs/udf/misc.c:205:
> >+       tag *tag_p;
> >            ^
> 
> I'd say "don't add any new typedefs" (and perhaps get rid of old ones).

It should be doing that at the site of the new typedef :)

-apw

Re: bug in checkpatch (on pointers to typedefs?)

[Jan Engelhardt]

On Feb 14 2008 10:06, Andy Whitcroft wrote:
>On Wed, Feb 13, 2008 at 08:43:58PM +0100, Jan Engelhardt wrote:
>> 
>> On Feb 10 2008 15:33, Marcin Slusarz wrote:
>> >Checkpatch in current mainline outputs following errors:
>> >
>> >$ ./scripts/checkpatch.pl -q --file ./fs/udf/misc.c
>> >ERROR: need consistent spacing around '*' (ctx:WxV)
>> >#205: FILE: fs/udf/misc.c:205:
>> >+       tag *tag_p;
>> >            ^
>> 
>> I'd say "don't add any new typedefs" (and perhaps get rid of old ones).
>
>It should be doing that at the site of the new typedef :)
>
Additionally, yes.
Given:

typedef struct {
	...
} tag_t;
void foo(void)
{
	tag_t name;
}

A user adding

+	tag_t newthing;

could at the same time give the struct a name if it already does not
have one and instead use

+	struct tag newthing;

then.