Skip to contentSkip to conversation

Add missing types to checkpatch.pl --list-types

PATCH Applied Jean Delvare 2 participants 9 messages Sep 2, 2017 – Sep 4, 2017 Checkpatch

lore
Share & tools
lore
#0 lore raw

cover Add missing types to checkpatch.pl --list-types

[PATCH 1/3] checkpatch: fix typo in comment
[PATCH 2/3] checkpatch: rename variables to avoid confusion [PATCH 3/3] checkpatch: add 6 missing types to --list-types

---
 scripts/checkpatch.pl |   37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)
Signature · 2 lines
Jean Delvare
SUSE L3 Support
#1 lore raw

PATCH 1/3 checkpatch: fix typo in comment

Cc Andy Whitcroft <[email protected]>
Cc Joe Perches <[email protected]>
---
 scripts/checkpatch.pl |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
@@ -2875,7 +2875,7 @@ sub process {
 #	#defines that are a single string
 #
 # There are 3 different line length message types:
-# LONG_LINE_COMMENT	a comment starts before but extends beyond $max_linelength
+# LONG_LINE_COMMENT	a comment starts before but extends beyond $max_line_length
 # LONG_LINE_STRING	a string starts before but extends beyond $max_line_length
 # LONG_LINE		all other lines longer than $max_line_length
 #
Signature · 2 lines
Jean Delvare
SUSE L3 Support
#2 lore raw

PATCH 2/3 checkpatch: rename variables to avoid confusion

In reply to Jean Delvare

The variable name "$msg_type" is sometimes used to set the message type, and sometimes used to set the message level. This works but is kind of confusing. Use "$msg_level" in the latter case instead, to make the code clearer.

Cc Andy Whitcroft <[email protected]>
Cc Joe Perches <[email protected]>
---
 scripts/checkpatch.pl |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
scripts/checkpatch.pl +16 -16
@@ -2715,10 +2715,10 @@ sub process {
 				my $typo_fix = $spelling_fix{lc($typo)};
 				$typo_fix = ucfirst($typo_fix) if ($typo =~ /^[A-Z]/);
 				$typo_fix = uc($typo_fix) if ($typo =~ /^[A-Z]+$/);
-				my $msg_type = \&WARN;
-				$msg_type = \&CHK if ($file);
-				if (&{$msg_type}("TYPO_SPELLING",
-						 "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
+				my $msg_level = \&WARN;
+				$msg_level = \&CHK if ($file);
+				if (&{$msg_level}("TYPO_SPELLING",
+						  "'$typo' may be misspelled - perhaps '$typo_fix'?\n" . $herecurr) &&
 				    $fix) {
 					$fixed[$fixlinenr] =~ s/(^|[^A-Za-z@])($typo)($|[^A-Za-z@])/$1$typo_fix$3/;
 				}
@@ -2753,10 +2753,10 @@ sub process {
 		    $rawline =~ /\b59\s+Temple\s+Pl/i ||
 		    $rawline =~ /\b51\s+Franklin\s+St/i) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
-			my $msg_type = \&ERROR;
-			$msg_type = \&CHK if ($file);
-			&{$msg_type}("FSF_MAILING_ADDRESS",
-				     "Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The FSF has changed addresses in the past, and may do so again. Linux already includes a copy of the GPL.\n" . $herevet)
+			my $msg_level = \&ERROR;
+			$msg_level = \&CHK if ($file);
+			&{$msg_level}("FSF_MAILING_ADDRESS",
+				      "Do not include the paragraph about writing to the Free Software Foundation's mailing address from the sample GPL notice. The FSF has changed addresses in the past, and may do so again. Linux already includes a copy of the GPL.\n" . $herevet)
 		}
 
 # check for Kconfig help text having a real description
@@ -3810,10 +3810,10 @@ sub process {
 
 # avoid BUG() or BUG_ON()
 		if ($line =~ /\b(?:BUG|BUG_ON)\b/) {
-			my $msg_type = \&WARN;
-			$msg_type = \&CHK if ($file);
-			&{$msg_type}("AVOID_BUG",
-				     "Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr);
+			my $msg_level = \&WARN;
+			$msg_level = \&CHK if ($file);
+			&{$msg_level}("AVOID_BUG",
+				      "Avoid crashing the kernel - try using WARN_ON & recovery code rather than BUG() or BUG_ON()\n" . $herecurr);
 		}
 
 # avoid LINUX_VERSION_CODE
@@ -4339,11 +4339,11 @@ sub process {
 
 					# messages are ERROR, but ?: are CHK
 					if ($ok == 0) {
-						my $msg_type = \&ERROR;
-						$msg_type = \&CHK if (($op eq '?:' || $op eq '?' || $op eq ':') && $ctx =~ /VxV/);
+						my $msg_level = \&ERROR;
+						$msg_level = \&CHK if (($op eq '?:' || $op eq '?' || $op eq ':') && $ctx =~ /VxV/);
 
-						if (&{$msg_type}("SPACING",
-								 "spaces required around that '$op' $at\n" . $hereptr)) {
+						if (&{$msg_level}("SPACING",
+								  "spaces required around that '$op' $at\n" . $hereptr)) {
 							$good = rtrim($fix_elements[$n]) . " " . trim($fix_elements[$n + 1]) . " ";
 							if (defined $fix_elements[$n + 2]) {
 								$fix_elements[$n + 2] =~ s/^\s+//;
Signature · 2 lines
Jean Delvare
SUSE L3 Support
#3 lore raw

PATCH 3/3 checkpatch: add 6 missing types to --list-types

In reply to Jean Delvare

Unlike all other types, LONG_LINE, LONG_LINE_COMMENT and LONG_LINE_STRING are passed to WARN() through a variable. This causes the parser in list_types() to miss them and consequently they are not present in the output of --list-types.

Additionally, types TYPO_SPELLING, FSF_MAILING_ADDRESS and AVOID_BUG are passed with a variable level, causing the parser to miss them too.

So modify the regex to also catch these special cases.

Fixes 3beb42eced39 ("checkpatch: add --list-types to show message types to show or ignore")
Cc Andy Whitcroft <[email protected]>
Cc Joe Perches <[email protected]>
---
 scripts/checkpatch.pl |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +2 -1
@@ -145,7 +145,8 @@ sub list_types {
 	close($script);
 
 	my @types = ();
-	for ($text =~ /\b(?:(?:CHK|WARN|ERROR)\s*\(\s*"([^"]+)")/g) {
+	# Also catch when type or level is passed through a variable
+	for ($text =~ /(?:(?:\bCHK|\bWARN|\bERROR|&\{\$msg_level})\s*\(|\$msg_type\s*=)\s*"([^"]+)"/g) {
 		push (@types, $_);
 	}
 	@types = sort(uniq(@types));
Signature · 2 lines
Jean Delvare
SUSE L3 Support
maintainer Acked-by on the v1 series
#4 lore raw
In reply to Jean Delvare
Quoted 3 lines · Jean Delvare

[PATCH 1/3] checkpatch: fix typo in comment
[PATCH 2/3] checkpatch: rename variables to avoid confusion [PATCH 3/3] checkpatch: add 6 missing types to --list-types

Andrew, can you please pick these up? Thanks.

Jean, that's a pretty obscure defect.
Thanks for handling it.

btw: how or why did you find it?

#5 lore raw

Hi Joe,

Quoted 9 lines · Joe Perches
Quoted 3 lines · Jean Delvare

[PATCH 1/3] checkpatch: fix typo in comment
[PATCH 2/3] checkpatch: rename variables to avoid confusion [PATCH 3/3] checkpatch: add 6 missing types to --list-types

Andrew, can you please pick these up? Thanks.

Jean, that's a pretty obscure defect.
Thanks for handling it.

You're welcome.

Quoted 1 line

btw: how or why did you find it?

I wrote a patch which introduces a lot of long lines, and I am fine with that in this specific case, so I wanted to ask checkpatch to print everything BUT line-over-80-columns warnings. I used --list-types to find out the internal name for it... and couldn't see anything matching in the list. But I knew it had to exist, so looked into the code.

Signature · 2 lines
Jean Delvare
SUSE L3 Support
#6 lore raw

Oh, btw, is there any purpose in listing a number in front of each type? It makes it look like one can pass that number instead of the type name, however it doesn't work, and I don't think it should as the numbering isn't stable and could change with any update of the script.

Can't we simplify the output and simply print the list of type names?

Thanks,

Signature · 2 lines
Jean Delvare
SUSE L3 Support
maintainer
#7 lore raw
Quoted 2 lines · Jean Delvare

Oh, btw, is there any purpose in listing a number in front of each type?

Not really. I did that because I wanted a header
and because I had no idea how many of those types
existed and I was too lazy to count.

Quoted 3 lines

It makes it look like one can pass that number instead of the type name, however it doesn't work, and I don't think it should as the numbering isn't stable and could change with any update of the script.

True.

Quoted 1 line

Can't we simplify the output and simply print the list of type names?

<shrug> If you want.

#8 lore raw
Quoted 7 lines · Joe Perches
Quoted 2 lines · Jean Delvare

Oh, btw, is there any purpose in listing a number in front of each type?

Not really. I did that because I wanted a header
and because I had no idea how many of those types
existed and I was too lazy to count.

Given the intended audience of the script, I think "| wc -l" is the way to answer that question.

Quoted 9 lines
Quoted 3 lines

It makes it look like one can pass that number instead of the type name, however it doesn't work, and I don't think it should as the numbering isn't stable and could change with any update of the script.

True.

Quoted 1 line

Can't we simplify the output and simply print the list of type names?

<shrug> If you want.

Patch coming.

Signature · 2 lines
Jean Delvare
SUSE L3 Support