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
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
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: [email protected]
---
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 <[email protected]> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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: [email protected]
> ---
> 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: [email protected]
---
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
On Feb. 11, 2008, 18:40 +0200, Andy Whitcroft <[email protected]> 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: [email protected]
>> ---
>> 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: [email protected]
> ---
> 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: [email protected]
---
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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
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
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
On Feb. 11, 2008, 20:42 +0200, Andy Whitcroft <[email protected]> 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
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).
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
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.