Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1725527 > unrolled thread

[PATCH 0/3] Add missing types to checkpatch.pl --list-types

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] Add missing types to checkpatch.pl --list-types Jean Delvare <jdelvare@suse.de> - 2017-09-02 18:00 +0200
    [PATCH 2/3] checkpatch: rename variables to avoid confusion Jean Delvare <jdelvare@suse.de> - 2017-09-02 18:00 +0200
    [PATCH 3/3] checkpatch: add 6 missing types to --list-types Jean Delvare <jdelvare@suse.de> - 2017-09-02 18:00 +0200
    Re: [PATCH 0/3] Add missing types to checkpatch.pl --list-types Joe Perches <joe@perches.com> - 2017-09-02 18:30 +0200
      Re: [PATCH 0/3] Add missing types to checkpatch.pl --list-types Jean Delvare <jdelvare@suse.de> - 2017-09-02 21:20 +0200
        Re: [PATCH 0/3] Add missing types to checkpatch.pl --list-types Jean Delvare <jdelvare@suse.de> - 2017-09-02 23:10 +0200
          Re: [PATCH 0/3] Add missing types to checkpatch.pl --list-types Joe Perches <joe@perches.com> - 2017-09-03 01:00 +0200
            Re: [PATCH 0/3] Add missing types to checkpatch.pl --list-types Jean Delvare <jdelvare@suse.de> - 2017-09-04 10:10 +0200

#1725527 — [PATCH 0/3] 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(-)
-- 
Jean Delvare
SUSE L3 Support

[toc] | [next] | [standalone]

#1725528 — [PATCH 2/3] checkpatch: rename variables to avoid confusion

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.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |   32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

--- linux-4.13-rc7.orig/scripts/checkpatch.pl	2017-09-02 17:21:04.490467219 +0200
+++ linux-4.13-rc7/scripts/checkpatch.pl	2017-09-02 17:23:28.899502742 +0200
@@ -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+//;

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1725531 — [PATCH 3/3] checkpatch: add 6 missing types to --list-types

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.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Fixes: 3beb42eced39 ("checkpatch: add --list-types to show message types to show or ignore")
Cc: Andy Whitcroft <apw@canonical.com>
Cc: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

--- linux-4.13-rc7.orig/scripts/checkpatch.pl	2017-09-02 17:46:45.852024037 +0200
+++ linux-4.13-rc7/scripts/checkpatch.pl	2017-09-02 17:46:55.925163818 +0200
@@ -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));

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1725533

On Sat, 2017-09-02 at 17:51 +0200, Jean Delvare wrote:
> [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?

Acked-by: Joe Perches <joe@perches.com>

[toc] | [prev] | [next] | [standalone]

#1725552

Hi Joe,

On Sat, 02 Sep 2017 09:25:29 -0700, Joe Perches wrote:
> On Sat, 2017-09-02 at 17:51 +0200, Jean Delvare wrote:
> > [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.

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

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1725582

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,
-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [next] | [standalone]

#1725584

On Sat, 2017-09-02 at 23:03 +0200, Jean Delvare wrote:
> 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.

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

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

<shrug>  If you want.

[toc] | [prev] | [next] | [standalone]

#1725915

On Sat, 02 Sep 2017 15:58:05 -0700, Joe Perches wrote:
> On Sat, 2017-09-02 at 23:03 +0200, Jean Delvare wrote:
> > 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.

> > 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.
> 
> > Can't we simplify the output and simply print the list of type names?  
> 
> <shrug>  If you want.

Patch coming.

-- 
Jean Delvare
SUSE L3 Support

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web