checkpatch: warn on missing spaces in broken up quoted strings
Subsystems (2)
PATCH 1/1 checkpatch: warn on missing spaces in broken up quoted strings
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).
scripts/checkpatch.pl +6 -0
@@ -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;
Joe Perches
maintainerscripts/checkpatch.pl
Quoted 21 lines · Dan Carpenter
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.hwhere 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 <[email protected]>scripts/checkpatch.pl +0
-0
[]
scripts/checkpatch.pl sub process { · line 4016
Quoted 5 lines
scripts/checkpatch.pl +5
-0+# 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]/
Quoted 8 lines · Joe Perches
scripts/checkpatch.pl sub process { · line 4016
Quoted 5 lines
scripts/checkpatch.pl +5
-0+# 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
Joe Perches
maintainerQuoted 9 lines · Dan Carpenter
Quoted 7 lines · Joe Perches
scripts/checkpatch.pl sub process { · line 4016
Quoted 5 lines
scripts/checkpatch.pl +5
-0+# 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.
[]
Quoted 4 lines
./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.
Quoted 3 lines
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
Quoted 31 lines · Joe Perches
Quoted 9 lines · Dan Carpenter
Quoted 7 lines · Joe Perches
scripts/checkpatch.pl sub process { · line 4016
Quoted 5 lines
scripts/checkpatch.pl +5
-0+# 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.[]
Quoted 4 lines
./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 positivesThese look more like defects to me.
Quoted 3 lines
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?
Quoted 8 lines
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
Joe Perches
maintainerQuoted 39 lines · Dan Carpenter
Quoted 38 lines · Joe Perches
Quoted 9 lines · Dan Carpenter
Quoted 7 lines · Joe Perches
scripts/checkpatch.pl sub process { · line 4016
Quoted 5 lines
scripts/checkpatch.pl +5
-0+# 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.[]
Quoted 4 lines
./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 positivesThese look more like defects to me.
Quoted 3 lines
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.
Quoted 6 lines
Quoted 1 line
(?:\\(?:[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
Quoted 12 lines · Joe Perches
Quoted 9 lines
Quoted 4 lines
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
PATCH v2 1/1 checkpatch: warn on missing spaces in broken up quoted
In reply to Joe PerchesCheckpatch 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.
---
v2: Move to correct spot in checkpatch
Fix indenting
Use "\w" instead of "[a-zA-Z]"scripts/checkpatch.pl +6 -0
@@ -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",