[PATCH 0/3] checkpatch: Miscellaneous cleanups

[PATCH 0/3] checkpatch: Miscellaneous cleanups

[Joe Perches]

Joe Perches (3):
  checkpatch: Add an error test for no space before comma
  checkpatch: Add error on use of attribute((weak)) or __weak
  checkpatch: Improve test for no space after cast

 scripts/checkpatch.pl | 34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)

-- 
2.1.0

[PATCH 1/3] checkpatch: Add an error test for no space before comma

[Joe Perches]

Using code like:

	int foo , bar;

is not preferred to:

	int foo, bar;

so emit an error on this style.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 374abf4..696254e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3563,14 +3563,33 @@ sub process {
 						}
 					}
 
-				# , must have a space on the right.
+				# , must not have a space before and must have a space on the right.
 				} elsif ($op eq ',') {
+					my $rtrim_before = 0;
+					my $space_after = 0;
+					if ($ctx =~ /Wx./) {
+						if (ERROR("SPACING",
+							  "space prohibited before that '$op' $at\n" . $hereptr)) {
+							$line_fixed = 1;
+							$rtrim_before = 1;
+						}
+					}
 					if ($ctx !~ /.x[WEC]/ && $cc !~ /^}/) {
 						if (ERROR("SPACING",
 							  "space required after that '$op' $at\n" . $hereptr)) {
-							$good = $fix_elements[$n] . trim($fix_elements[$n + 1]) . " ";
 							$line_fixed = 1;
 							$last_after = $n;
+							$space_after = 1;
+						}
+					}
+					if ($rtrim_before || $space_after) {
+						if ($rtrim_before) {
+							$good = rtrim($fix_elements[$n]) . trim($fix_elements[$n + 1]);
+						} else {
+							$good = $fix_elements[$n] . trim($fix_elements[$n + 1]);
+						}
+						if ($space_after) {
+							$good .= " ";
 						}
 					}
 
-- 
2.1.0

[PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak

[Joe Perches]

Using weak can have unintended link defects.
Emit an error on its use.

Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 696254e..692fdb6 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4671,6 +4671,14 @@ sub process {
 			}
 		}
 
+# Check for __attribute__ weak, or __weak (may have link issues)
+		if ($realfile !~ m@\binclude/uapi/@ &&
+		    ($line =~ /\b__attribute__\s*\(\s*\(.*\bweak\b/ ||
+		     $line =~ \b__weak\b/)) {
+			ERROR("AVOID_WEAK",
+			      "Using weak can have unintended link defects" . $herecurr);
+		}
+
 # check for sizeof(&)
 		if ($line =~ /\bsizeof\s*\(\s*\&/) {
 			WARN("SIZEOF_ADDRESS",
-- 
2.1.0

Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak

[Andrew Morton]

On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:

> Using weak can have unintended link defects.
> Emit an error on its use.

Well, we don't want a warning about use of __weak in function
definitions.  Only in declarations.

> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4671,6 +4671,14 @@ sub process {
>  			}
>  		}
>  
> +# Check for __attribute__ weak, or __weak (may have link issues)
> +		if ($realfile !~ m@\binclude/uapi/@ &&

And this bit maybe is checking for use in a header file, which is not
as good as checking for a declaration but is probably good enough. 
However the "uapi" bit is confusing.

Can we have a better changelog please?

> +		    ($line =~ /\b__attribute__\s*\(\s*\(.*\bweak\b/ ||
> +		     $line =~ \b__weak\b/)) {
> +			ERROR("AVOID_WEAK",
> +			      "Using weak can have unintended link defects" . $herecurr);
> +		}
> +
>  # check for sizeof(&)
>  		if ($line =~ /\bsizeof\s*\(\s*\&/) {
>  			WARN("SIZEOF_ADDRESS",
> -- 
> 2.1.0

Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak

[Joe Perches]

On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > Using weak can have unintended link defects.
> > Emit an error on its use.
> 
> Well, we don't want a warning about use of __weak in function
> definitions.  Only in declarations.

Why is that?

> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -4671,6 +4671,14 @@ sub process {
> >  			}
> >  		}
> >  
> > +# Check for __attribute__ weak, or __weak (may have link issues)
> > +		if ($realfile !~ m@\binclude/uapi/@ &&
> 
> And this bit maybe is checking for use in a header file, which is not
> as good as checking for a declaration but is probably good enough. 
> However the "uapi" bit is confusing.
> 
> Can we have a better changelog please?

Suggestions?

Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak

[Andrew Morton]

On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <joe@perches.com> wrote:

> On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> > On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > Using weak can have unintended link defects.
> > > Emit an error on its use.
> > 
> > Well, we don't want a warning about use of __weak in function
> > definitions.  Only in declarations.
> 
> Why is that?

Because the problem we're trying to detect is when __weak is used on a
declaration.

This is OK:

foo.h:
extern int foo(void);
foo.c:
int __weak foo(void)
{
	...
}

But this is not OK:

foo.h:
extern __weak int foo(void);
foo.c:
int __weak foo(void)
{
	...
}

Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak

[Joe Perches]

On Wed, 2014-10-15 at 12:50 -0700, Andrew Morton wrote:
> On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <joe@perches.com> wrote:
> 
> > On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> > > On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> > > 
> > > > Using weak can have unintended link defects.
> > > > Emit an error on its use.
> > > 
> > > Well, we don't want a warning about use of __weak in function
> > > definitions.  Only in declarations.
> > 
> > Why is that?
> 
> Because the problem we're trying to detect is when __weak is used on a
> declaration.
> 
> This is OK:
> 
> foo.h:
> extern int foo(void);
> foo.c:
> int __weak foo(void)
> {
> 	...
> }
> 
> But this is not OK:
> 
> foo.h:
> extern __weak int foo(void);
> foo.c:
> int __weak foo(void)
> {
> 	...
> }
> 

And this?

foo.c:

extern __weak int foo(void);

int __weak foo(void)
{
}

Re: [PATCH 2/3] checkpatch: Add error on use of attribute((weak)) or __weak

[Andrew Morton]

On Wed, 15 Oct 2014 12:52:44 -0700 Joe Perches <joe@perches.com> wrote:

> On Wed, 2014-10-15 at 12:50 -0700, Andrew Morton wrote:
> > On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <joe@perches.com> wrote:
> > 
> > > On Wed, 2014-10-15 at 12:42 -0700, Andrew Morton wrote:
> > > > On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <joe@perches.com> wrote:
> > > > 
> > > > > Using weak can have unintended link defects.
> > > > > Emit an error on its use.
> > > > 
> > > > Well, we don't want a warning about use of __weak in function
> > > > definitions.  Only in declarations.
> > > 
> > > Why is that?
> > 
> > Because the problem we're trying to detect is when __weak is used on a
> > declaration.
> > 
> > This is OK:
> > 
> > foo.h:
> > extern int foo(void);
> > foo.c:
> > int __weak foo(void)
> > {
> > 	...
> > }
> > 
> > But this is not OK:
> > 
> > foo.h:
> > extern __weak int foo(void);
> > foo.c:
> > int __weak foo(void)
> > {
> > 	...
> > }
> > 
> 
> And this?
> 
> foo.c:
> 
> extern __weak int foo(void);
> 
> int __weak foo(void)
> {
> }
> 

That's why I just said "And this bit maybe is checking for use in a
header file, which is not as good as checking for a declaration but is
probably good enough."

I don't think that would trigger the bug anyway.  The problem is that

extern __weak int foo(void);

int foo(void)
{
}

unexpectedly and undesirably turns foo() into __weak.

I think it would be sufficient to check for __weak in a declaration. 
If that isn't practical then checking for __weak in a .h file should
suffice.

[PATCH 2/3 V2] [PATCH] checkpatch: Add error on use of attribute((weak)) or __weak declarations

[Joe Perches]

Using weak declarations can have unintended link defects.
Emit an error on its use.

Signed-off-by: Joe Perches <joe@perches.com>
---
Maybe you prefer this.
It only warns on declarations.

 scripts/checkpatch.pl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 696254e..8a577aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4671,6 +4671,15 @@ sub process {
 			}
 		}
 
+# Check for __attribute__ weak, or __weak declarations (may have link issues)
+		if ($^V && $^V ge 5.10.0 &&
+		    $line =~ /(?:$Declare|$DeclareMisordered)\s*$Ident\s*$balanced_parens\s*(?:$Attribute)?\s*;/ &&
+		    ($line =~ /\b__attribute__\s*\(\s*\(.*\bweak\b/ ||
+		     $line =~ /\b__weak\b/)) {
+			ERROR("WEAK_DECLARATION",
+			      "Using weak declarations can have unintended link defects\n" . $herecurr);
+		}
+
 # check for sizeof(&)
 		if ($line =~ /\bsizeof\s*\(\s*\&/) {
 			WARN("SIZEOF_ADDRESS",
-- 
2.1.0

[PATCH 3/3] checkpatch: Improve test for no space after cast

[Joe Perches]

sizeof(foo) is not a cast, allow a space after it.

Signed-off-by: Joe Perches <joe@perches.com>
Tested-by: Kalle Valo <kvalo@qca.qualcomm.com>
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 692fdb6..21dac46 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2515,7 +2515,8 @@ sub process {
 			}
 		}
 
-		if ($line =~ /^\+.*\(\s*$Type\s*\)[ \t]+(?!$Assignment|$Arithmetic|{)/) {
+		if ($line =~ /^\+.*(\w+\s*)?\(\s*$Type\s*\)[ \t]+(?!$Assignment|$Arithmetic|[,;\({\[\<\>])/ &&
+		    (!defined($1) || $1 !~ /sizeof\s*/)) {
 			if (CHK("SPACING",
 				"No space is necessary after a cast\n" . $herecurr) &&
 			    $fix) {
-- 
2.1.0