* Bug in checkpatch.pl @ 2010-11-02 22:57 Audun Hoem 2010-11-03 12:04 ` [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling Florian Mickler 0 siblings, 1 reply; 7+ messages in thread From: Audun Hoem @ 2010-11-02 22:57 UTC (permalink / raw) To: Andy Whitcroft; +Cc: linux-kernel I have stumbled about a bug in checkpatch.pl while working on some code in drivers/staging. It seems to get confused when confronted with asterisks. For example, this snippe: kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL); Here the asterisk is in it's binary form, obviously, and performs a multiplication, however checkpatch reports this: drivers/staging/frontier/alphatrack.c:772: ERROR: space prohibited after that '*' (ctx:WxW) So it's obviously thinking it's the unary operator, which should only be preceded by a variable name or another unary operator such as ++. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling 2010-11-02 22:57 Bug in checkpatch.pl Audun Hoem @ 2010-11-03 12:04 ` Florian Mickler 2010-11-03 15:20 ` Florian Mickler 0 siblings, 1 reply; 7+ messages in thread From: Florian Mickler @ 2010-11-03 12:04 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Audun Hoem, linux-kernel, Florian Mickler Casts have to be handled after the last type that is followed by an opening parenthesis is handled. On Tue, 2 Nov 2010 23:57:36 +0100 Audun Hoem <audun.hoem@gmail.com> wrote: > I have stumbled about a bug in checkpatch.pl while working on some > code in drivers/staging. It seems to get confused when confronted with > asterisks. For example, this snippe: > > kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL); > > Here the asterisk is in it's binary form, obviously, and performs a > multiplication, however checkpatch reports this: > > drivers/staging/frontier/alphatrack.c:772: ERROR: space prohibited > after that '*' (ctx:WxW) > > So it's obviously thinking it's the unary operator, which should only > be preceded by a variable name or another unary operator such as ++. Reported-By: Audun Hoem <audun.hoem@gmail.com> Signed-off-by: Florian Mickler <florian@mickler.org> --- scripts/checkpatch.pl | 11 +++++++---- 1 files changed, 7 insertions(+), 4 deletions(-) diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 90b54d4..c1cbb09 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -859,10 +859,6 @@ sub annotate_values { $av_preprocessor = 0; } - } elsif ($cur =~ /^(\(\s*$Type\s*)\)/) { - print "CAST($1)\n" if ($dbg_values > 1); - push(@av_paren_type, $type); - $type = 'C'; } elsif ($cur =~ /^($Type)\s*(?:$Ident|,|\)|\(|\s*$)/) { print "DECLARE($1)\n" if ($dbg_values > 1); @@ -963,6 +959,13 @@ sub annotate_values { $type = 'V'; $av_pending = 'V'; + } elsif ($cur =~ /^(\(\s*$Type\s*)\)/) { + #casts handled after last type that opens a brace + #is handled, else it screws up the parens handling + print "CAST($1)\n" if ($dbg_values > 1); + push(@av_paren_type, $type); + $type = 'C'; + } elsif ($cur =~ /^($Ident\s*):(?:\s*\d+\s*(,|=|;))?/) { if (defined $2 && $type eq 'C' || $type eq 'T') { $av_pend_colon = 'B'; -- 1.7.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling 2010-11-03 12:04 ` [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling Florian Mickler @ 2010-11-03 15:20 ` Florian Mickler 2010-11-03 15:26 ` Audun Hoem 2010-11-17 7:30 ` Florian Mickler 0 siblings, 2 replies; 7+ messages in thread From: Florian Mickler @ 2010-11-03 15:20 UTC (permalink / raw) To: Florian Mickler; +Cc: Andy Whitcroft, Audun Hoem, linux-kernel On Wed, 3 Nov 2010 13:04:33 +0100 Florian Mickler <florian@mickler.org> wrote: > Casts have to be handled after the last type that is followed by an > opening parenthesis is handled. That is the wrong fix. I realized now that with that patch we would not claim anything as a CAST anymore. Better is probably to only claim a CAST if av_pending is not set. Andy, would that work? It seems to be alright... Do you have some tests for checkpatch? Testing it with the reported line and some other random samples it seems to be alright. Regards, Flo >8------------------------------------------------------------------------ commit 11ed611c647420ea27124faf269d724b4fd3ade4 Author: Florian Mickler <florian@mickler.org> Date: Wed Nov 3 15:54:26 2010 +0100 checkpatch.pl: fix CAST detection We should only claim that something is a cast if we did not encouter a token before, that did set av_pending. This fixes the operator * in the line below to be detected as binary (vs unary). kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL); Reported-by: Audun Hoem <audun.hoem@gmail.com> Signed-off-by: Florian Mickler <florian@mickler.org> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 90b54d4..06f5c44 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -859,7 +859,7 @@ sub annotate_values { $av_preprocessor = 0; } - } elsif ($cur =~ /^(\(\s*$Type\s*)\)/) { + } elsif ($cur =~ /^(\(\s*$Type\s*)\)/ && $av_pending eq '_') { print "CAST($1)\n" if ($dbg_values > 1); push(@av_paren_type, $type); $type = 'C'; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling 2010-11-03 15:20 ` Florian Mickler @ 2010-11-03 15:26 ` Audun Hoem 2010-11-03 16:07 ` Florian Mickler 2010-11-17 7:30 ` Florian Mickler 1 sibling, 1 reply; 7+ messages in thread From: Audun Hoem @ 2010-11-03 15:26 UTC (permalink / raw) To: Florian Mickler; +Cc: Andy Whitcroft, linux-kernel On 11/3/10, Florian Mickler <florian@mickler.org> wrote: > On Wed, 3 Nov 2010 13:04:33 +0100 > Florian Mickler <florian@mickler.org> wrote: > >> Casts have to be handled after the last type that is followed by an >> opening parenthesis is handled. > > That is the wrong fix. I realized now that with that patch we would > not claim anything as a CAST anymore. Better is probably to only claim > a CAST if av_pending is not set. Andy, would that work? It seems to be > alright... Do you have some tests for checkpatch? > > Testing it with the reported line and some other random samples it > seems to be alright. > Probably good enough of a test to try it on random kernel code. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling 2010-11-03 15:26 ` Audun Hoem @ 2010-11-03 16:07 ` Florian Mickler 0 siblings, 0 replies; 7+ messages in thread From: Florian Mickler @ 2010-11-03 16:07 UTC (permalink / raw) To: Audun Hoem; +Cc: Andy Whitcroft, linux-kernel On Wed, 3 Nov 2010 16:26:44 +0100 Audun Hoem <audun.hoem@gmail.com> wrote: > On 11/3/10, Florian Mickler <florian@mickler.org> wrote: > > On Wed, 3 Nov 2010 13:04:33 +0100 > > Florian Mickler <florian@mickler.org> wrote: > > > >> Casts have to be handled after the last type that is followed by an > >> opening parenthesis is handled. > > > > That is the wrong fix. I realized now that with that patch we would > > not claim anything as a CAST anymore. Better is probably to only claim > > a CAST if av_pending is not set. Andy, would that work? It seems to be > > alright... Do you have some tests for checkpatch? > > > > Testing it with the reported line and some other random samples it > > seems to be alright. > > > > Probably good enough of a test to try it on random kernel code. Yes, on random inspection of kernel code the new output looks good. (I've set dbg_values to 2 and diffed the output for some files of the patched version with the output of the broken version) I speculated on Andy to have a special set of problematic test cases, but I looked through the git-log of checkpatch and tested some of the stuff there. It all seems ok. Regards, Flo ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling 2010-11-03 15:20 ` Florian Mickler 2010-11-03 15:26 ` Audun Hoem @ 2010-11-17 7:30 ` Florian Mickler 2010-11-23 14:15 ` Andy Whitcroft 1 sibling, 1 reply; 7+ messages in thread From: Florian Mickler @ 2010-11-17 7:30 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Audun Hoem, linux-kernel Hi Andy, did you get this? Regards, Flo On Wed, 3 Nov 2010 16:20:42 +0100 Florian Mickler <florian@mickler.org> wrote: > On Wed, 3 Nov 2010 13:04:33 +0100 > Florian Mickler <florian@mickler.org> wrote: > > > Casts have to be handled after the last type that is followed by an > > opening parenthesis is handled. > > That is the wrong fix. I realized now that with that patch we would > not claim anything as a CAST anymore. Better is probably to only claim > a CAST if av_pending is not set. Andy, would that work? It seems to be > alright... Do you have some tests for checkpatch? > > Testing it with the reported line and some other random samples it > seems to be alright. > > Regards, > Flo > > >8------------------------------------------------------------------------ > commit 11ed611c647420ea27124faf269d724b4fd3ade4 > Author: Florian Mickler <florian@mickler.org> > Date: Wed Nov 3 15:54:26 2010 +0100 > > checkpatch.pl: fix CAST detection > > We should only claim that something is a cast if we did not encouter a > token before, that did set av_pending. > > This fixes the operator * in the line below to be detected as > binary (vs unary). > > kmalloc(sizeof(struct alphatrack_ocmd) * true_size, GFP_KERNEL); > > Reported-by: Audun Hoem <audun.hoem@gmail.com> > Signed-off-by: Florian Mickler <florian@mickler.org> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 90b54d4..06f5c44 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -859,7 +859,7 @@ sub annotate_values { > $av_preprocessor = 0; > } > > - } elsif ($cur =~ /^(\(\s*$Type\s*)\)/) { > + } elsif ($cur =~ /^(\(\s*$Type\s*)\)/ && $av_pending eq '_') { > print "CAST($1)\n" if ($dbg_values > 1); > push(@av_paren_type, $type); > $type = 'C'; ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling 2010-11-17 7:30 ` Florian Mickler @ 2010-11-23 14:15 ` Andy Whitcroft 0 siblings, 0 replies; 7+ messages in thread From: Andy Whitcroft @ 2010-11-23 14:15 UTC (permalink / raw) To: Florian Mickler; +Cc: Audun Hoem, linux-kernel On Wed, Nov 17, 2010 at 08:30:31AM +0100, Florian Mickler wrote: > > Hi Andy, > > did you get this? Yep sorry, need to get these hooverered up into my tree. -apw ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2010-11-23 14:15 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2010-11-02 22:57 Bug in checkpatch.pl Audun Hoem 2010-11-03 12:04 ` [PATCH] checkpatch.pl: fix CAST detection to not screw with parens handling Florian Mickler 2010-11-03 15:20 ` Florian Mickler 2010-11-03 15:26 ` Audun Hoem 2010-11-03 16:07 ` Florian Mickler 2010-11-17 7:30 ` Florian Mickler 2010-11-23 14:15 ` Andy Whitcroft
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).