* [patch] checkpatch: warn on missing spaces in broken up quoted strings @ 2014-06-13 6:53 Dan Carpenter 2014-06-13 9:30 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2014-06-13 6:53 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Joe Perches, linux-kernel Checkpatch already complains when people break up quoted strings but it's still pretty common. One mistake that people often make is they leave out the space character between the two strings. This check adds 453 new warnings. There very few false positives, here is what they look like: 1) Most of the false positives are in crypto/testmgr.h where they just want a 10x10 block of sample text and don't care about the content. 2) There one commented place like this: "das08-aoh" "das08-aol" 3) There is one place which breaks the alphabet at the lower and upper case boundary. 4) There is one person who broke quoted strings at the 80 character mark without considering the content (that's not really a false positive, now that I think about it). Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 010b18e..c50eee2 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4009,6 +4009,12 @@ sub process { } } +# check for missing a space in a string concatination + if ($prevrawline =~ /[^\\][a-zA-Z]"$/ && $rawline =~ /^\+[\t ]+"[a-zA-Z]/) { + WARN('MISSING_SPACE', + "break quoted strings at a space character\n" . $hereprev); + } + # check for bad placement of section $InitAttribute (e.g.: __initdata) if ($line =~ /(\b$InitAttribute\b)/) { my $attr = $1; ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings 2014-06-13 6:53 [patch] checkpatch: warn on missing spaces in broken up quoted strings Dan Carpenter @ 2014-06-13 9:30 ` Joe Perches 2014-06-13 9:46 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2014-06-13 9:30 UTC (permalink / raw) To: Dan Carpenter; +Cc: Andy Whitcroft, linux-kernel On Fri, 2014-06-13 at 09:53 +0300, Dan Carpenter wrote: > Checkpatch already complains when people break up quoted strings but > it's still pretty common. One mistake that people often make is they > leave out the space character between the two strings. > > This check adds 453 new warnings. There very few false positives, here > is what they look like: > > 1) Most of the false positives are in crypto/testmgr.h where they just > want a 10x10 block of sample text and don't care about the content. > 2) There one commented place like this: > "das08-aoh" > "das08-aol" > 3) There is one place which breaks the alphabet at the lower and upper > case boundary. > 4) There is one person who broke quoted strings at the 80 character mark > without considering the content (that's not really a false positive, > now that I think about it). > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > +# check for missing a space in a string concatination > + if ($prevrawline =~ /[^\\][a-zA-Z]"$/ && $rawline =~ /^\+[\t ]+"[a-zA-Z]/) { > + WARN('MISSING_SPACE', > + "break quoted strings at a space character\n" . $hereprev); > + } Probably want digits too so maybe \w instead of "[a-zA-Z]/ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings 2014-06-13 9:30 ` Joe Perches @ 2014-06-13 9:46 ` Dan Carpenter 2014-06-13 19:52 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2014-06-13 9:46 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel On Fri, Jun 13, 2014 at 02:30:22AM -0700, Joe Perches wrote: > > +# check for missing a space in a string concatination > > + if ($prevrawline =~ /[^\\][a-zA-Z]"$/ && $rawline =~ /^\+[\t ]+"[a-zA-Z]/) { > > + WARN('MISSING_SPACE', > > + "break quoted strings at a space character\n" . $hereprev); > > + } > > Probably want digits too so maybe \w instead of "[a-zA-Z]/ > Changing it to that adds the following 6 new warnings: ./drivers/staging/media/lirc/lirc_zilog.c:806 "1) -- please upgrade to a newer driver", - bug. ./drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c:529 "24G_A[%d] = 0x%x\n", i, ./drivers/staging/rtl8192e/rtl8192e/r8192E_dev.c:537 "24G_C[%d] = 0x%x\n", i, - the "24G_A" is the last 5 characters of a variable name so this is sloppy code. ./drivers/scsi/pm8001/pm8001_ctl.c:297 "0x%08x 0x%08x\n", ./drivers/scsi/pm8001/pm8001_ctl.c:432 "0x%08x 0x%08x\n", ./drivers/scsi/qla2xxx/qla_nx2.c:1457 "0x%X 0x%X 0x%X 0x%X 0x%X 0x%X\n" - hex false positives I thought about doing that when I wrote my original patch but I was worried about more hex false positives. If there are only those 3 then it's probably not a big deal. What do you think? regards, dan carpenter ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings 2014-06-13 9:46 ` Dan Carpenter @ 2014-06-13 19:52 ` Joe Perches 2014-06-16 6:40 ` Dan Carpenter 0 siblings, 1 reply; 9+ messages in thread From: Joe Perches @ 2014-06-13 19:52 UTC (permalink / raw) To: Dan Carpenter; +Cc: Andy Whitcroft, linux-kernel On Fri, 2014-06-13 at 12:46 +0300, Dan Carpenter wrote: > On Fri, Jun 13, 2014 at 02:30:22AM -0700, Joe Perches wrote: > > > > +# check for missing a space in a string concatination > > > + if ($prevrawline =~ /[^\\][a-zA-Z]"$/ && $rawline =~ /^\+[\t ]+"[a-zA-Z]/) { > > > + WARN('MISSING_SPACE', > > > + "break quoted strings at a space character\n" . $hereprev); > > > + } > > > > Probably want digits too so maybe \w instead of "[a-zA-Z]/ Couple nits: The indentation isn't right here. And this check is probably better placed immediately after the "SPLIT_STRING" test. [] > ./drivers/scsi/pm8001/pm8001_ctl.c:297 "0x%08x 0x%08x\n", > ./drivers/scsi/pm8001/pm8001_ctl.c:432 "0x%08x 0x%08x\n", > ./drivers/scsi/qla2xxx/qla_nx2.c:1457 "0x%X 0x%X 0x%X 0x%X 0x%X 0x%X\n" > - hex false positives These look more like defects to me. > I thought about doing that when I wrote my original patch but I was > worried about more hex false positives. If there are only those 3 then > it's probably not a big deal. What do you think? I know it works on files, but I'm not sure it works on patches. What happens when checking a single line replacement patch? Likely the \\[a-zA-Z] check should include all the tests that the multiple line string exceptions use. (?:\\(?:[ntr]|[0-7]{1,3}|x[0-9a-fA-F]{1,2})|;\s*|\{\s*)"\s*$ cheers, Joe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings 2014-06-13 19:52 ` Joe Perches @ 2014-06-16 6:40 ` Dan Carpenter 2014-06-16 18:27 ` Joe Perches 0 siblings, 1 reply; 9+ messages in thread From: Dan Carpenter @ 2014-06-16 6:40 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel On Fri, Jun 13, 2014 at 12:52:25PM -0700, Joe Perches wrote: > On Fri, 2014-06-13 at 12:46 +0300, Dan Carpenter wrote: > > On Fri, Jun 13, 2014 at 02:30:22AM -0700, Joe Perches wrote: > > > > > > +# check for missing a space in a string concatination > > > > + if ($prevrawline =~ /[^\\][a-zA-Z]"$/ && $rawline =~ /^\+[\t ]+"[a-zA-Z]/) { > > > > + WARN('MISSING_SPACE', > > > > + "break quoted strings at a space character\n" . $hereprev); > > > > + } > > > > > > Probably want digits too so maybe \w instead of "[a-zA-Z]/ > > Couple nits: > > The indentation isn't right here. > > And this check is probably better placed immediately > after the "SPLIT_STRING" test. > > [] > > > ./drivers/scsi/pm8001/pm8001_ctl.c:297 "0x%08x 0x%08x\n", > > ./drivers/scsi/pm8001/pm8001_ctl.c:432 "0x%08x 0x%08x\n", > > ./drivers/scsi/qla2xxx/qla_nx2.c:1457 "0x%X 0x%X 0x%X 0x%X 0x%X 0x%X\n" > > - hex false positives > > These look more like defects to me. > > > I thought about doing that when I wrote my original patch but I was > > worried about more hex false positives. If there are only those 3 then > > it's probably not a big deal. What do you think? > > I know it works on files, but I'm not sure it works on patches. > > What happens when checking a single line replacement patch? > > Likely the \\[a-zA-Z] check should include > all the tests that the multiple line string exceptions use. > > (?:\\(?:[ntr]|[0-7]{1,3}|x[0-9a-fA-F]{1,2})|;\s*|\{\s*)"\s*$ That kind of complicate regex hurts my head. How do I make it not complain about: "foo\n\t" "bar"; regards, dan carpenter ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings 2014-06-16 6:40 ` Dan Carpenter @ 2014-06-16 18:27 ` Joe Perches 2014-07-23 11:56 ` Dan Carpenter 2014-07-23 12:11 ` Dan Carpenter 0 siblings, 2 replies; 9+ messages in thread From: Joe Perches @ 2014-06-16 18:27 UTC (permalink / raw) To: Dan Carpenter; +Cc: Andy Whitcroft, linux-kernel On Mon, 2014-06-16 at 09:40 +0300, Dan Carpenter wrote: > On Fri, Jun 13, 2014 at 12:52:25PM -0700, Joe Perches wrote: > > On Fri, 2014-06-13 at 12:46 +0300, Dan Carpenter wrote: > > > On Fri, Jun 13, 2014 at 02:30:22AM -0700, Joe Perches wrote: > > > > > > > > +# check for missing a space in a string concatination > > > > > + if ($prevrawline =~ /[^\\][a-zA-Z]"$/ && $rawline =~ /^\+[\t ]+"[a-zA-Z]/) { > > > > > + WARN('MISSING_SPACE', > > > > > + "break quoted strings at a space character\n" . $hereprev); > > > > > + } > > > > > > > > Probably want digits too so maybe \w instead of "[a-zA-Z]/ > > > > Couple nits: > > > > The indentation isn't right here. > > > > And this check is probably better placed immediately > > after the "SPLIT_STRING" test. > > > > [] > > > > > ./drivers/scsi/pm8001/pm8001_ctl.c:297 "0x%08x 0x%08x\n", > > > ./drivers/scsi/pm8001/pm8001_ctl.c:432 "0x%08x 0x%08x\n", > > > ./drivers/scsi/qla2xxx/qla_nx2.c:1457 "0x%X 0x%X 0x%X 0x%X 0x%X 0x%X\n" > > > - hex false positives > > > > These look more like defects to me. > > > > > I thought about doing that when I wrote my original patch but I was > > > worried about more hex false positives. If there are only those 3 then > > > it's probably not a big deal. What do you think? > > > > I know it works on files, but I'm not sure it works on patches. > > > > What happens when checking a single line replacement patch? > > > > Likely the \\[a-zA-Z] check should include > > all the tests that the multiple line string exceptions use. > > > > (?:\\(?:[ntr]|[0-7]{1,3}|x[0-9a-fA-F]{1,2})|;\s*|\{\s*)"\s*$ > > That kind of complicate regex hurts my head. How do I make it not > complain about: > "foo\n\t" > "bar"; Did you try it? The first test should handle any \n, \t or \r at the end of a string. [0-7]{1,3] is for octal x[0-9afAF]{1,2} is for hex constants ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings 2014-06-16 18:27 ` Joe Perches @ 2014-07-23 11:56 ` Dan Carpenter 2014-07-23 12:11 ` Dan Carpenter 1 sibling, 0 replies; 9+ messages in thread From: Dan Carpenter @ 2014-07-23 11:56 UTC (permalink / raw) To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel On Mon, Jun 16, 2014 at 11:27:36AM -0700, Joe Perches wrote: > > > Likely the \\[a-zA-Z] check should include > > > all the tests that the multiple line string exceptions use. > > > > > > (?:\\(?:[ntr]|[0-7]{1,3}|x[0-9a-fA-F]{1,2})|;\s*|\{\s*)"\s*$ > > > > That kind of complicate regex hurts my head. How do I make it not > > complain about: > > "foo\n\t" > > "bar"; > > Did you try it? > Of course I tried it... I certainly am not able to *read* it. :P I think that I don't know what you meant? Just give me a whole patch I can apply. I also tested my version against patches and not just whole files and it seems to work fine. regards, dan carpenter ^ permalink raw reply [flat|nested] 9+ messages in thread
* [patch v2] checkpatch: warn on missing spaces in broken up quoted 2014-06-16 18:27 ` Joe Perches @ 2014-07-23 12:11 ` Dan Carpenter 2014-07-23 12:11 ` Dan Carpenter 1 sibling, 0 replies; 9+ messages in thread From: Dan Carpenter @ 2014-07-23 12:11 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Joe Perches, linux-kernel, kernel-janitors Checkpatch already complains when people break up quoted strings but it's still pretty common. One mistake that people often make is they leave out the space character between the two strings. This check adds around 450 new warnings and has a low rate of false positives. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: Move to correct spot in checkpatch Fix indenting Use "\w" instead of "[a-zA-Z]" diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a0880ed..132e6e2 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2361,6 +2361,12 @@ sub process { "quoted string split across lines\n" . $hereprev); } +# check for missing a space in a string concatination + if ($prevrawline =~ /[^\\]\w"$/ && $rawline =~ /^\+[\t ]+"\w/) { + WARN('MISSING_SPACE', + "break quoted strings at a space character\n" . $hereprev); + } + # check for spaces before a quoted newline if ($rawline =~ /^.*\".*\s\\n/) { if (WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE", ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [patch v2] checkpatch: warn on missing spaces in broken up quoted @ 2014-07-23 12:11 ` Dan Carpenter 0 siblings, 0 replies; 9+ messages in thread From: Dan Carpenter @ 2014-07-23 12:11 UTC (permalink / raw) To: Andy Whitcroft; +Cc: Joe Perches, linux-kernel, kernel-janitors Checkpatch already complains when people break up quoted strings but it's still pretty common. One mistake that people often make is they leave out the space character between the two strings. This check adds around 450 new warnings and has a low rate of false positives. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: Move to correct spot in checkpatch Fix indenting Use "\w" instead of "[a-zA-Z]" diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index a0880ed..132e6e2 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2361,6 +2361,12 @@ sub process { "quoted string split across lines\n" . $hereprev); } +# check for missing a space in a string concatination + if ($prevrawline =~ /[^\\]\w"$/ && $rawline =~ /^\+[\t ]+"\w/) { + WARN('MISSING_SPACE', + "break quoted strings at a space character\n" . $hereprev); + } + # check for spaces before a quoted newline if ($rawline =~ /^.*\".*\s\\n/) { if (WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE", ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2014-07-23 12:11 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-06-13 6:53 [patch] checkpatch: warn on missing spaces in broken up quoted strings Dan Carpenter 2014-06-13 9:30 ` Joe Perches 2014-06-13 9:46 ` Dan Carpenter 2014-06-13 19:52 ` Joe Perches 2014-06-16 6:40 ` Dan Carpenter 2014-06-16 18:27 ` Joe Perches 2014-07-23 11:56 ` Dan Carpenter 2014-07-23 12:11 ` [patch v2] checkpatch: warn on missing spaces in broken up quoted Dan Carpenter 2014-07-23 12:11 ` Dan Carpenter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.