[patch] checkpatch: warn on missing spaces in broken up quoted strings

[patch] checkpatch: warn on missing spaces in broken up quoted strings

[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.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;

Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings

[Joe Perches]

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]/

Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings

[Dan Carpenter]

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

Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings

[Joe Perches]

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

Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings

[Dan Carpenter]

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

Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings

[Joe Perches]

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

Re: [patch] checkpatch: warn on missing spaces in broken up quoted strings

[Dan Carpenter]

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

[patch v2] checkpatch: warn on missing spaces in broken up quoted

[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 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",

[patch v2] checkpatch: warn on missing spaces in broken up quoted

[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 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",