Skip to contentSkip to conversation

checkpatch: Miscellaneous cleanups

PATCH In review Joe Perches 2 participants 10 messages Oct 15, 2014 Checkpatch

lore
Share & tools
lore

Joe Perches

author maintainer
#0 lore raw

cover checkpatch: Miscellaneous cleanups

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(-)
Signature · 1 line
2.1.0
author maintainer
#1 lore raw

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

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 <[email protected]>
---
 scripts/checkpatch.pl | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +21 -2
index 374abf4..696254e 100755
@@ -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 .= " ";
 						}
 					}
Signature · 1 line
2.1.0
author maintainer
#2 lore raw

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

In reply to Joe Perches

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

Signed-off-by Joe Perches <[email protected]>
---
 scripts/checkpatch.pl | 8 ++++++++
 1 file changed, 8 insertions(+)
scripts/checkpatch.pl +8 -0
index 696254e..692fdb6 100755
@@ -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",
Signature · 1 line
2.1.0
#3 lore raw

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

Quoted 2 lines · On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <[email protected]> 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.

scripts/checkpatch.pl sub process { · line 4675

Quoted 8 lines
scripts/checkpatch.pl +2 -0
@@ -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?

Quoted 11 lines
scripts/checkpatch.pl +6 -0
+		    ($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",
Signature · 1 line
2.1.0
author maintainer
#4 lore raw

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

Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <[email protected]> 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?

Quoted 6 lines
Quoted 6 lines
scripts/checkpatch.pl +0 -0
@@ -4671,6 +4671,14 @@ sub process {
 			}
 		}
Quoted 8 lines

scripts/checkpatch.pl sub process { · line 4675

Quoted 2 lines
scripts/checkpatch.pl +2 -0
+# 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?

#5 lore raw

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

Quoted 10 lines · On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <[email protected]> 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)

{
	...
}
author maintainer
#6 lore raw

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

Quoted 28 lines · Andrew Morton
Quoted 10 lines · On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <[email protected]> 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:

Quoted 8 lines

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)
{
}

#7 lore raw

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

Quoted 40 lines · On Wed, 15 Oct 2014 12:52:44 -0700 Joe Perches <[email protected]> wrote:
Quoted 28 lines · Andrew Morton
Quoted 10 lines · On Wed, 15 Oct 2014 12:45:48 -0700 Joe Perches <[email protected]> wrote:
Quoted 7 lines · Andrew Morton
Quoted 2 lines · On Wed, 15 Oct 2014 12:32:08 -0700 Joe Perches <[email protected]> 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:

Quoted 8 lines

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

{
	...
}

And this?

Quoted 8 lines

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.

author maintainer
#8 lore raw

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

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

Signed-off-by Joe Perches <[email protected]>

---
Maybe you prefer this.
It only warns on declarations.

 scripts/checkpatch.pl | 9 +++++++++
 1 file changed, 9 insertions(+)
scripts/checkpatch.pl +9 -0
index 696254e..8a577aa 100755
@@ -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",
Signature · 1 line
2.1.0
author maintainer
#9 lore raw

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

In reply to Joe Perches

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

Signed-off-by Joe Perches <[email protected]>
Tested-by Kalle Valo <[email protected]>
---
 scripts/checkpatch.pl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +2 -1
index 692fdb6..21dac46 100755
@@ -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) {
Signature · 1 line
2.1.0