LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/4] checkpatch: fix multiple const * types
@ 2019-05-08 12:27 Antonio Borneo
  2019-05-08 12:27 ` [PATCH 2/4] checkpatch: add --fix for warning LINE_CONTINUATIONS Antonio Borneo
                   ` (3 more replies)
  0 siblings, 4 replies; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 12:27 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

Commit 1574a29f8e76 ("checkpatch: allow multiple const * types")
claims to support repetition of pattern "const *", but it actually
allows only one extra instance.
Check the following lines
	int a(char const * const x[]);
	int b(char const * const *x);
	int c(char const * const * const x[]);
	int d(char const * const * const *x);
with command
	./scripts/checkpatch.pl --show-types -f filename
to find that only the first line passes the test, while a warning
is triggered by the other 3 lines:
	WARNING:FUNCTION_ARGUMENTS: function definition argument
	'char const * const' should also have an identifier name
The reason is that the pattern match halts at the second asterisk
in the line, thus the remaining text starting with asterisk fails
to match a valid name for a variable.

Fixed by replacing "?" (Match 1 or 0 times) with "*" (Match 0 or
more times) in the regular expression.
Fix also the similar test for types in unusual order.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Fixes: 1574a29f8e76 ("checkpatch: allow multiple const * types")
---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a09333fd7cef..f40d4bb2fbb9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -802,12 +802,12 @@ sub build_types {
 		  }x;
 	$Type	= qr{
 			$NonptrType
-			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)?
+			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)*
 			(?:\s+$Inline|\s+$Modifier)*
 		  }x;
 	$TypeMisordered	= qr{
 			$NonptrTypeMisordered
-			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)?
+			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)*
 			(?:\s+$Inline|\s+$Modifier)*
 		  }x;
 	$Declare	= qr{(?:$Storage\s+(?:$Inline\s+)?)?$Type};
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 2/4] checkpatch: add --fix for warning LINE_CONTINUATIONS
  2019-05-08 12:27 [PATCH 1/4] checkpatch: fix multiple const * types Antonio Borneo
@ 2019-05-08 12:27 ` Antonio Borneo
  2019-05-08 14:19   ` Joe Perches
  2019-05-08 12:27 ` [PATCH 3/4] checkpatch: fix minor typo and mixed space+tab in indentation Antonio Borneo
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 12:27 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

The warning LINE_CONTINUATIONS does not offer a --fix.

Add the trivial --fix.
In case of consecutive lines with the same issue, this will
fix only the first line.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---
 scripts/checkpatch.pl | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f40d4bb2fbb9..9a247183b65c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5207,8 +5207,11 @@ sub process {
 			    $line !~ /^\+\s*\#.*\\$/ &&		# preprocessor
 			    $line !~ /^\+.*\b(__asm__|asm)\b.*\\$/ &&	# asm
 			    $line =~ /^\+.*\\$/) {
-				WARN("LINE_CONTINUATIONS",
-				     "Avoid unnecessary line continuations\n" . $herecurr);
+				if (WARN("LINE_CONTINUATIONS",
+					 "Avoid unnecessary line continuations\n" . $herecurr) &&
+				    $fix) {
+					$fixed[$fixlinenr] =~ s/\s*\\$//;
+				}
 			}
 		}
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 3/4] checkpatch: fix minor typo and mixed space+tab in indentation
  2019-05-08 12:27 [PATCH 1/4] checkpatch: fix multiple const * types Antonio Borneo
  2019-05-08 12:27 ` [PATCH 2/4] checkpatch: add --fix for warning LINE_CONTINUATIONS Antonio Borneo
@ 2019-05-08 12:27 ` Antonio Borneo
  2020-01-22 16:38   ` [PATCH V2] " Antonio Borneo
  2019-05-08 12:27 ` [PATCH 4/4] checkpatch: replace magic value for TAB size Antonio Borneo
  2019-05-08 14:51 ` [PATCH 1/4] checkpatch: fix multiple const * types Joe Perches
  3 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 12:27 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

Fix spelling of "concatenation".
Don't use tab after space in indentation.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---
 scripts/checkpatch.pl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 9a247183b65c..373ad345f732 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4546,7 +4546,7 @@ sub process {
 					    ($op eq '>' &&
 					     $ca =~ /<\S+\@\S+$/))
 					{
-					    	$ok = 1;
+						$ok = 1;
 					}
 
 					# for asm volatile statements
@@ -4881,7 +4881,7 @@ sub process {
 			# conditional.
 			substr($s, 0, length($c), '');
 			$s =~ s/\n.*//g;
-			$s =~ s/$;//g; 	# Remove any comments
+			$s =~ s/$;//g;	# Remove any comments
 			if (length($c) && $s !~ /^\s*{?\s*\\*\s*$/ &&
 			    $c !~ /}\s*while\s*/)
 			{
@@ -4920,7 +4920,7 @@ sub process {
 # if and else should not have general statements after it
 		if ($line =~ /^.\s*(?:}\s*)?else\b(.*)/) {
 			my $s = $1;
-			$s =~ s/$;//g; 	# Remove any comments
+			$s =~ s/$;//g;	# Remove any comments
 			if ($s !~ /^\s*(?:\sif|(?:{|)\s*\\?\s*$)/) {
 				ERROR("TRAILING_STATEMENTS",
 				      "trailing statements should be on next line\n" . $herecurr);
@@ -5095,7 +5095,7 @@ sub process {
 			{
 			}
 
-			# Flatten any obvious string concatentation.
+			# Flatten any obvious string concatenation.
 			while ($dstat =~ s/($String)\s*$Ident/$1/ ||
 			       $dstat =~ s/$Ident\s*($String)/$1/)
 			{
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH 4/4] checkpatch: replace magic value for TAB size
  2019-05-08 12:27 [PATCH 1/4] checkpatch: fix multiple const * types Antonio Borneo
  2019-05-08 12:27 ` [PATCH 2/4] checkpatch: add --fix for warning LINE_CONTINUATIONS Antonio Borneo
  2019-05-08 12:27 ` [PATCH 3/4] checkpatch: fix minor typo and mixed space+tab in indentation Antonio Borneo
@ 2019-05-08 12:27 ` Antonio Borneo
  2019-05-08 14:52   ` Joe Perches
  2019-05-08 14:51 ` [PATCH 1/4] checkpatch: fix multiple const * types Joe Perches
  3 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 12:27 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

The size of 8 characters used for both TAB and indentation is
embedded as magic value allover the checkpatch script, and this
makes the script less readable.

Replace the magic value 8 with the perl variable "$tabsize".
From the context of the code it's clear if it is used for
indentation or tabulation, so no need to use two separate
variables.

As side benefit of this change, it makes easy to replace the TAB
size when this script is used by other projects with different
requirements in their coding style (e.g. OpenOCD [1] requires
TAB size of 4 characters [2]).
In these cases the script will be probably modified and adapted,
so there is no need (at least for the moment) to expose this
setting on the script's command line.

[1] http://openocd.org/
[2] http://openocd.org/doc/doxygen/html/stylec.html#styleformat

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Signed-off-by: Erik Ahlén <erik.ahlen@avalonenterprise.com>
Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>
---
 scripts/checkpatch.pl | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 373ad345f732..3c8115600516 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -62,6 +62,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
+my $tabsize = 8;
 
 sub help {
 	my ($exitcode) = @_;
@@ -1211,7 +1212,7 @@ sub expand_tabs {
 		if ($c eq "\t") {
 			$res .= ' ';
 			$n++;
-			for (; ($n % 8) != 0; $n++) {
+			for (; ($n % $tabsize) != 0; $n++) {
 				$res .= ' ';
 			}
 			next;
@@ -2224,7 +2225,7 @@ sub string_find_replace {
 sub tabify {
 	my ($leading) = @_;
 
-	my $source_indent = 8;
+	my $source_indent = $tabsize;
 	my $max_spaces_before_tab = $source_indent - 1;
 	my $spaces_to_tab = " " x $source_indent;
 
@@ -3153,7 +3154,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
-# more than 8 must use tabs.
+# more than $tabsize must use tabs.
 		if ($rawline =~ /^\+\s* \t\s*\S/ ||
 		    $rawline =~ /^\+\s*        \s*/) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
@@ -3172,7 +3173,7 @@ sub process {
 				"please, no space before tabs\n" . $herevet) &&
 			    $fix) {
 				while ($fixed[$fixlinenr] =~
-					   s/(^\+.*) {8,8}\t/$1\t\t/) {}
+					   s/(^\+.*) {$tabsize,$tabsize}\t/$1\t\t/) {}
 				while ($fixed[$fixlinenr] =~
 					   s/(^\+.*) +\t/$1\t/) {}
 			}
@@ -3194,11 +3195,11 @@ sub process {
 		if ($perl_version_ok &&
 		    $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) {
 			my $indent = length($1);
-			if ($indent % 8) {
+			if ($indent % $tabsize) {
 				if (WARN("TABSTOP",
 					 "Statements should start on a tabstop\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
+					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/$tabsize)@e;
 				}
 			}
 		}
@@ -3216,8 +3217,8 @@ sub process {
 				my $newindent = $2;
 
 				my $goodtabindent = $oldindent .
-					"\t" x ($pos / 8) .
-					" "  x ($pos % 8);
+					"\t" x ($pos / $tabsize) .
+					" "  x ($pos % $tabsize);
 				my $goodspaceindent = $oldindent . " "  x $pos;
 
 				if ($newindent ne $goodtabindent &&
@@ -3688,11 +3689,11 @@ sub process {
 			#print "line<$line> prevline<$prevline> indent<$indent> sindent<$sindent> check<$check> continuation<$continuation> s<$s> cond_lines<$cond_lines> stat_real<$stat_real> stat<$stat>\n";
 
 			if ($check && $s ne '' &&
-			    (($sindent % 8) != 0 ||
+			    (($sindent % $tabsize) != 0 ||
 			     ($sindent < $indent) ||
 			     ($sindent == $indent &&
 			      ($s !~ /^\s*(?:\}|\{|else\b)/)) ||
-			     ($sindent > $indent + 8))) {
+			     ($sindent > $indent + $tabsize))) {
 				WARN("SUSPECT_CODE_INDENT",
 				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 2/4] checkpatch: add --fix for warning LINE_CONTINUATIONS
  2019-05-08 12:27 ` [PATCH 2/4] checkpatch: add --fix for warning LINE_CONTINUATIONS Antonio Borneo
@ 2019-05-08 14:19   ` Joe Perches
  0 siblings, 0 replies; 27+ messages in thread
From: Joe Perches @ 2019-05-08 14:19 UTC (permalink / raw)
  To: Antonio Borneo, Andy Whitcroft; +Cc: linux-kernel

On Wed, 2019-05-08 at 14:27 +0200, Antonio Borneo wrote:
> The warning LINE_CONTINUATIONS does not offer a --fix.
> 
> Add the trivial --fix.
> In case of consecutive lines with the same issue, this will
> fix only the first line.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -5207,8 +5207,11 @@ sub process {
>  			    $line !~ /^\+\s*\#.*\\$/ &&		# preprocessor
>  			    $line !~ /^\+.*\b(__asm__|asm)\b.*\\$/ &&	# asm
>  			    $line =~ /^\+.*\\$/) {
> -				WARN("LINE_CONTINUATIONS",
> -				     "Avoid unnecessary line continuations\n" . $herecurr);
> +				if (WARN("LINE_CONTINUATIONS",
> +					 "Avoid unnecessary line continuations\n" . $herecurr) &&

I prefer to not apply this.

Problem here is that these should generally be fixed by hand
as the automated fix isn't very good style either.

> +				    $fix) {
> +					$fixed[$fixlinenr] =~ s/\s*\\$//;
> +				}
>  			}
>  		}
>  


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/4] checkpatch: fix multiple const * types
  2019-05-08 12:27 [PATCH 1/4] checkpatch: fix multiple const * types Antonio Borneo
                   ` (2 preceding siblings ...)
  2019-05-08 12:27 ` [PATCH 4/4] checkpatch: replace magic value for TAB size Antonio Borneo
@ 2019-05-08 14:51 ` Joe Perches
  2019-05-08 15:26   ` Antonio Borneo
  2019-05-08 17:43   ` [PATCH v2] " Antonio Borneo
  3 siblings, 2 replies; 27+ messages in thread
From: Joe Perches @ 2019-05-08 14:51 UTC (permalink / raw)
  To: Antonio Borneo, Andy Whitcroft; +Cc: linux-kernel

On Wed, 2019-05-08 at 14:27 +0200, Antonio Borneo wrote:
> Commit 1574a29f8e76 ("checkpatch: allow multiple const * types")
> claims to support repetition of pattern "const *", but it actually
> allows only one extra instance.
> Check the following lines
> 	int a(char const * const x[]);
> 	int b(char const * const *x);
> 	int c(char const * const * const x[]);
> 	int d(char const * const * const *x);
> with command
> 	./scripts/checkpatch.pl --show-types -f filename
> to find that only the first line passes the test, while a warning
> is triggered by the other 3 lines:
> 	WARNING:FUNCTION_ARGUMENTS: function definition argument
> 	'char const * const' should also have an identifier name
> The reason is that the pattern match halts at the second asterisk
> in the line, thus the remaining text starting with asterisk fails
> to match a valid name for a variable.
> 
> Fixed by replacing "?" (Match 1 or 0 times) with "*" (Match 0 or
> more times) in the regular expression.
> Fix also the similar test for types in unusual order.

It might be better to use a max match like {0,4} instead of *

perl is pretty memory intensive at multiple unrestricted matches
of somewhat complex patterns.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/4] checkpatch: replace magic value for TAB size
  2019-05-08 12:27 ` [PATCH 4/4] checkpatch: replace magic value for TAB size Antonio Borneo
@ 2019-05-08 14:52   ` Joe Perches
  2019-05-08 15:32     ` Antonio Borneo
  0 siblings, 1 reply; 27+ messages in thread
From: Joe Perches @ 2019-05-08 14:52 UTC (permalink / raw)
  To: Antonio Borneo, Andy Whitcroft; +Cc: linux-kernel

On Wed, 2019-05-08 at 14:27 +0200, Antonio Borneo wrote:
> The size of 8 characters used for both TAB and indentation is
> embedded as magic value allover the checkpatch script, and this
> makes the script less readable.
> 
> Replace the magic value 8 with the perl variable "$tabsize".
> From the context of the code it's clear if it is used for
> indentation or tabulation, so no need to use two separate
> variables.
> 
> As side benefit of this change, it makes easy to replace the TAB
> size when this script is used by other projects with different
> requirements in their coding style (e.g. OpenOCD [1] requires
> TAB size of 4 characters [2]).
> In these cases the script will be probably modified and adapted,
> so there is no need (at least for the moment) to expose this
> setting on the script's command line.

Disagree.  Probably getter to add a --tabsize=<foo> option now.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 1/4] checkpatch: fix multiple const * types
  2019-05-08 14:51 ` [PATCH 1/4] checkpatch: fix multiple const * types Joe Perches
@ 2019-05-08 15:26   ` Antonio Borneo
  2020-01-22 16:38     ` [PATCH V3] " Antonio Borneo
  2019-05-08 17:43   ` [PATCH v2] " Antonio Borneo
  1 sibling, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 15:26 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel

On Wed, May 8, 2019 at 4:51 PM Joe Perches <joe@perches.com> wrote:
...
> It might be better to use a max match like {0,4} instead of *
>
> perl is pretty memory intensive at multiple unrestricted matches
> of somewhat complex patterns.

Agree! Will send a V2!
Thanks for the review!

Antonio

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/4] checkpatch: replace magic value for TAB size
  2019-05-08 14:52   ` Joe Perches
@ 2019-05-08 15:32     ` Antonio Borneo
  2019-05-08 17:02       ` Joe Perches
  0 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 15:32 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel

On Wed, May 8, 2019 at 4:52 PM Joe Perches <joe@perches.com> wrote:
...
> > In these cases the script will be probably modified and adapted,
> > so there is no need (at least for the moment) to expose this
> > setting on the script's command line.
>
> Disagree.  Probably getter to add a --tabsize=<foo> option now.

Ok, will send a V2 including the command line option.
Exposing TAB size, makes the option name relevant; should I keep
"--tabsize" or is "--tab-stop" more appropriate?

Antonio

^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH 4/4] checkpatch: replace magic value for TAB size
  2019-05-08 15:32     ` Antonio Borneo
@ 2019-05-08 17:02       ` Joe Perches
  0 siblings, 0 replies; 27+ messages in thread
From: Joe Perches @ 2019-05-08 17:02 UTC (permalink / raw)
  To: Antonio Borneo; +Cc: Andy Whitcroft, linux-kernel

On Wed, 2019-05-08 at 17:32 +0200, Antonio Borneo wrote:
> On Wed, May 8, 2019 at 4:52 PM Joe Perches <joe@perches.com> wrote:
> ...
> > > In these cases the script will be probably modified and adapted,
> > > so there is no need (at least for the moment) to expose this
> > > setting on the script's command line.
> > 
> > Disagree.  Probably getter to add a --tabsize=<foo> option now.
> 
> Ok, will send a V2 including the command line option.
> Exposing TAB size, makes the option name relevant; should I keep
> "--tabsize" or is "--tab-stop" more appropriate?

--tabsize is probably more appropriate as tab stops are not
always a multiple of a single number.

Unless you really want to get funky and support something like
--tab-stops=7,13,17,...

I don't suggest that.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2] checkpatch: fix multiple const * types
@ 2019-05-08 17:43   ` Antonio Borneo
  2019-09-04  9:37     ` Antonio Borneo
  0 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 17:43 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

Commit 1574a29f8e76 ("checkpatch: allow multiple const * types")
claims to support repetition of pattern "const *", but it actually
allows only one extra instance.
Check the following lines
	int a(char const * const x[]);
	int b(char const * const *x);
	int c(char const * const * const x[]);
	int d(char const * const * const *x);
with command
	./scripts/checkpatch.pl --show-types -f filename
to find that only the first line passes the test, while a warning
is triggered by the other 3 lines:
	WARNING:FUNCTION_ARGUMENTS: function definition argument
	'char const * const' should also have an identifier name
The reason is that the pattern match halts at the second asterisk
in the line, thus the remaining text starting with asterisk fails
to match a valid name for a variable.

Fixed by replacing "?" (Match 1 or 0 times) with "{0,4}" (Match
no more than 4 times) in the regular expression.
Fix also the similar test for types in unusual order.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Fixes: 1574a29f8e76 ("checkpatch: allow multiple const * types")
---
v1->v2
	use a max match {0,4} instead of *, to limit the memory
	used by perl

 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a09333fd7cef..916a3fbd4d47 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -802,12 +802,12 @@ sub build_types {
 		  }x;
 	$Type	= qr{
 			$NonptrType
-			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)?
+			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+){0,4}
 			(?:\s+$Inline|\s+$Modifier)*
 		  }x;
 	$TypeMisordered	= qr{
 			$NonptrTypeMisordered
-			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)?
+			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+){0,4}
 			(?:\s+$Inline|\s+$Modifier)*
 		  }x;
 	$Declare	= qr{(?:$Storage\s+(?:$Inline\s+)?)?$Type};
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v2] checkpatch: add command-line option for TAB size
@ 2019-05-08 17:43 Antonio Borneo
  2019-05-08 17:56 ` Joe Perches
  2019-05-08 21:14 ` [PATCH v2] " Elliott, Robert (Servers)
  0 siblings, 2 replies; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 17:43 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

The size of 8 characters used for both TAB and indentation is
embedded as magic value allover the checkpatch script, and this
makes the script less readable.

Replace the magic value 8 with a variable.
From the context of the code it's clear if it is used for
indentation or tabulation, so no need to use two separate
variables.

Add a command-line option "--tab-size" to let the user select a
TAB size value other than 8.
This makes easy to reuse this script by other projects with
different requirements in their coding style (e.g. OpenOCD [1]
requires TAB size of 4 characters [2]).

[1] http://openocd.org/
[2] http://openocd.org/doc/doxygen/html/stylec.html#styleformat

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Signed-off-by: Erik Ahlén <erik.ahlen@avalonenterprise.com>
Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>
---
V1 -> V2
	add the command line option

 scripts/checkpatch.pl | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 916a3fbd4d47..90f641bf1895 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -62,6 +62,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
+my $tabsize = 8;
 
 sub help {
 	my ($exitcode) = @_;
@@ -96,6 +97,7 @@ Options:
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
   --min-conf-desc-length=n   set the min description length, if shorter, warn
+  --tab-size=n               set the number of spaces for tab (default 8)
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
   --mailback                 only produce a report in case of warnings/errors
@@ -213,6 +215,7 @@ GetOptions(
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
+	'tab-size=i'	=> \$tabsize,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
@@ -1211,7 +1214,7 @@ sub expand_tabs {
 		if ($c eq "\t") {
 			$res .= ' ';
 			$n++;
-			for (; ($n % 8) != 0; $n++) {
+			for (; ($n % $tabsize) != 0; $n++) {
 				$res .= ' ';
 			}
 			next;
@@ -2224,7 +2227,7 @@ sub string_find_replace {
 sub tabify {
 	my ($leading) = @_;
 
-	my $source_indent = 8;
+	my $source_indent = $tabsize;
 	my $max_spaces_before_tab = $source_indent - 1;
 	my $spaces_to_tab = " " x $source_indent;
 
@@ -3153,7 +3156,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
-# more than 8 must use tabs.
+# more than $tabsize must use tabs.
 		if ($rawline =~ /^\+\s* \t\s*\S/ ||
 		    $rawline =~ /^\+\s*        \s*/) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
@@ -3172,7 +3175,7 @@ sub process {
 				"please, no space before tabs\n" . $herevet) &&
 			    $fix) {
 				while ($fixed[$fixlinenr] =~
-					   s/(^\+.*) {8,8}\t/$1\t\t/) {}
+					   s/(^\+.*) {$tabsize,$tabsize}\t/$1\t\t/) {}
 				while ($fixed[$fixlinenr] =~
 					   s/(^\+.*) +\t/$1\t/) {}
 			}
@@ -3194,11 +3197,11 @@ sub process {
 		if ($perl_version_ok &&
 		    $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) {
 			my $indent = length($1);
-			if ($indent % 8) {
+			if ($indent % $tabsize) {
 				if (WARN("TABSTOP",
 					 "Statements should start on a tabstop\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
+					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/$tabsize)@e;
 				}
 			}
 		}
@@ -3216,8 +3219,8 @@ sub process {
 				my $newindent = $2;
 
 				my $goodtabindent = $oldindent .
-					"\t" x ($pos / 8) .
-					" "  x ($pos % 8);
+					"\t" x ($pos / $tabsize) .
+					" "  x ($pos % $tabsize);
 				my $goodspaceindent = $oldindent . " "  x $pos;
 
 				if ($newindent ne $goodtabindent &&
@@ -3688,11 +3691,11 @@ sub process {
 			#print "line<$line> prevline<$prevline> indent<$indent> sindent<$sindent> check<$check> continuation<$continuation> s<$s> cond_lines<$cond_lines> stat_real<$stat_real> stat<$stat>\n";
 
 			if ($check && $s ne '' &&
-			    (($sindent % 8) != 0 ||
+			    (($sindent % $tabsize) != 0 ||
 			     ($sindent < $indent) ||
 			     ($sindent == $indent &&
 			      ($s !~ /^\s*(?:\}|\{|else\b)/)) ||
-			     ($sindent > $indent + 8))) {
+			     ($sindent > $indent + $tabsize))) {
 				WARN("SUSPECT_CODE_INDENT",
 				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] checkpatch: add command-line option for TAB size
  2019-05-08 17:43 [PATCH v2] checkpatch: add command-line option for TAB size Antonio Borneo
@ 2019-05-08 17:56 ` Joe Perches
  2019-05-08 18:19   ` Antonio Borneo
  2019-05-08 21:14 ` [PATCH v2] " Elliott, Robert (Servers)
  1 sibling, 1 reply; 27+ messages in thread
From: Joe Perches @ 2019-05-08 17:56 UTC (permalink / raw)
  To: Antonio Borneo, Andy Whitcroft; +Cc: linux-kernel

On Wed, 2019-05-08 at 19:43 +0200, Antonio Borneo wrote:
> The size of 8 characters used for both TAB and indentation is
> embedded as magic value allover the checkpatch script, and this
> makes the script less readable.

I doubt this bit of the commit message is proper.

Tabs _are_ 8 in the linux-kernel sources and checkpatch
was written for the linux-kernel.

Using a variable _could_ reasonably be described as an
improvement, but readability wasn't and isn't really an
issue here.

Other than that, the patch seems fine.

thanks,  Joe

> Replace the magic value 8 with a variable.
> From the context of the code it's clear if it is used for
> indentation or tabulation, so no need to use two separate
> variables.
> 
> Add a command-line option "--tab-size" to let the user select a
> TAB size value other than 8.
> This makes easy to reuse this script by other projects with
> different requirements in their coding style (e.g. OpenOCD [1]
> requires TAB size of 4 characters [2]).
> 
> [1] http://openocd.org/
> [2] http://openocd.org/doc/doxygen/html/stylec.html#styleformat
> 
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
> Signed-off-by: Erik Ahlén <erik.ahlen@avalonenterprise.com>
> Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>
> ---
> V1 -> V2
> 	add the command line option
> 
>  scripts/checkpatch.pl | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 916a3fbd4d47..90f641bf1895 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -62,6 +62,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
>  my $typedefsfile = "";
>  my $color = "auto";
>  my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
> +my $tabsize = 8;
>  
>  sub help {
>  	my ($exitcode) = @_;
> @@ -96,6 +97,7 @@ Options:
>    --show-types               show the specific message type in the output
>    --max-line-length=n        set the maximum line length, if exceeded, warn
>    --min-conf-desc-length=n   set the min description length, if shorter, warn
> +  --tab-size=n               set the number of spaces for tab (default 8)
>    --root=PATH                PATH to the kernel tree root
>    --no-summary               suppress the per-file summary
>    --mailback                 only produce a report in case of warnings/errors
> @@ -213,6 +215,7 @@ GetOptions(
>  	'list-types!'	=> \$list_types,
>  	'max-line-length=i' => \$max_line_length,
>  	'min-conf-desc-length=i' => \$min_conf_desc_length,
> +	'tab-size=i'	=> \$tabsize,
>  	'root=s'	=> \$root,
>  	'summary!'	=> \$summary,
>  	'mailback!'	=> \$mailback,
> @@ -1211,7 +1214,7 @@ sub expand_tabs {
>  		if ($c eq "\t") {
>  			$res .= ' ';
>  			$n++;
> -			for (; ($n % 8) != 0; $n++) {
> +			for (; ($n % $tabsize) != 0; $n++) {
>  				$res .= ' ';
>  			}
>  			next;
> @@ -2224,7 +2227,7 @@ sub string_find_replace {
>  sub tabify {
>  	my ($leading) = @_;
>  
> -	my $source_indent = 8;
> +	my $source_indent = $tabsize;
>  	my $max_spaces_before_tab = $source_indent - 1;
>  	my $spaces_to_tab = " " x $source_indent;
>  
> @@ -3153,7 +3156,7 @@ sub process {
>  		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
>  
>  # at the beginning of a line any tabs must come first and anything
> -# more than 8 must use tabs.
> +# more than $tabsize must use tabs.
>  		if ($rawline =~ /^\+\s* \t\s*\S/ ||
>  		    $rawline =~ /^\+\s*        \s*/) {
>  			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
> @@ -3172,7 +3175,7 @@ sub process {
>  				"please, no space before tabs\n" . $herevet) &&
>  			    $fix) {
>  				while ($fixed[$fixlinenr] =~
> -					   s/(^\+.*) {8,8}\t/$1\t\t/) {}
> +					   s/(^\+.*) {$tabsize,$tabsize}\t/$1\t\t/) {}
>  				while ($fixed[$fixlinenr] =~
>  					   s/(^\+.*) +\t/$1\t/) {}
>  			}
> @@ -3194,11 +3197,11 @@ sub process {
>  		if ($perl_version_ok &&
>  		    $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) {
>  			my $indent = length($1);
> -			if ($indent % 8) {
> +			if ($indent % $tabsize) {
>  				if (WARN("TABSTOP",
>  					 "Statements should start on a tabstop\n" . $herecurr) &&
>  				    $fix) {
> -					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
> +					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/$tabsize)@e;
>  				}
>  			}
>  		}
> @@ -3216,8 +3219,8 @@ sub process {
>  				my $newindent = $2;
>  
>  				my $goodtabindent = $oldindent .
> -					"\t" x ($pos / 8) .
> -					" "  x ($pos % 8);
> +					"\t" x ($pos / $tabsize) .
> +					" "  x ($pos % $tabsize);
>  				my $goodspaceindent = $oldindent . " "  x $pos;
>  
>  				if ($newindent ne $goodtabindent &&
> @@ -3688,11 +3691,11 @@ sub process {
>  			#print "line<$line> prevline<$prevline> indent<$indent> sindent<$sindent> check<$check> continuation<$continuation> s<$s> cond_lines<$cond_lines> stat_real<$stat_real> stat<$stat>\n";
>  
>  			if ($check && $s ne '' &&
> -			    (($sindent % 8) != 0 ||
> +			    (($sindent % $tabsize) != 0 ||
>  			     ($sindent < $indent) ||
>  			     ($sindent == $indent &&
>  			      ($s !~ /^\s*(?:\}|\{|else\b)/)) ||
> -			     ($sindent > $indent + 8))) {
> +			     ($sindent > $indent + $tabsize))) {
>  				WARN("SUSPECT_CODE_INDENT",
>  				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
>  			}


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] checkpatch: add command-line option for TAB size
  2019-05-08 17:56 ` Joe Perches
@ 2019-05-08 18:19   ` Antonio Borneo
  2019-05-08 18:36     ` [PATCH v3] " Antonio Borneo
  0 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 18:19 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andy Whitcroft, linux-kernel

On Wed, May 8, 2019 at 7:56 PM Joe Perches <joe@perches.com> wrote:
>
> On Wed, 2019-05-08 at 19:43 +0200, Antonio Borneo wrote:
> > The size of 8 characters used for both TAB and indentation is
> > embedded as magic value allover the checkpatch script, and this
> > makes the script less readable.
>
> I doubt this bit of the commit message is proper.
>
> Tabs _are_ 8 in the linux-kernel sources and checkpatch
> was written for the linux-kernel.
>
> Using a variable _could_ reasonably be described as an
> improvement, but readability wasn't and isn't really an
> issue here.

Well, it depends on own skill with regular expressions in perl :-)

I will change the commit message focusing on the new command line flag
and drop this part.

Thanks
Antonio

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v3] checkpatch: add command-line option for TAB size
@ 2019-05-08 18:36     ` Antonio Borneo
  0 siblings, 0 replies; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 18:36 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

Linux kernel coding style requires a size of 8 characters for
both TAB and indentation, and such value is embedded as magic
value allover the checkpatch script.
This makes hard to reuse the script by other projects with
different requirements in their coding style (e.g. OpenOCD [1]
requires TAB size of 4 characters [2]).

Replace the magic value 8 with a variable.

Add a command-line option "--tab-size" to let the user select a
TAB size value other than 8.

[1] http://openocd.org/
[2] http://openocd.org/doc/doxygen/html/stylec.html#styleformat

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Signed-off-by: Erik Ahlén <erik.ahlen@avalonenterprise.com>
Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>
---
v1 -> v2
	add the command line option

v2 -> v3
	rewrite commit msg to remove script readability issue

 scripts/checkpatch.pl | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 916a3fbd4d47..90f641bf1895 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -62,6 +62,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
+my $tabsize = 8;
 
 sub help {
 	my ($exitcode) = @_;
@@ -96,6 +97,7 @@ Options:
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
   --min-conf-desc-length=n   set the min description length, if shorter, warn
+  --tab-size=n               set the number of spaces for tab (default 8)
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
   --mailback                 only produce a report in case of warnings/errors
@@ -213,6 +215,7 @@ GetOptions(
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
+	'tab-size=i'	=> \$tabsize,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
@@ -1211,7 +1214,7 @@ sub expand_tabs {
 		if ($c eq "\t") {
 			$res .= ' ';
 			$n++;
-			for (; ($n % 8) != 0; $n++) {
+			for (; ($n % $tabsize) != 0; $n++) {
 				$res .= ' ';
 			}
 			next;
@@ -2224,7 +2227,7 @@ sub string_find_replace {
 sub tabify {
 	my ($leading) = @_;
 
-	my $source_indent = 8;
+	my $source_indent = $tabsize;
 	my $max_spaces_before_tab = $source_indent - 1;
 	my $spaces_to_tab = " " x $source_indent;
 
@@ -3153,7 +3156,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
-# more than 8 must use tabs.
+# more than $tabsize must use tabs.
 		if ($rawline =~ /^\+\s* \t\s*\S/ ||
 		    $rawline =~ /^\+\s*        \s*/) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
@@ -3172,7 +3175,7 @@ sub process {
 				"please, no space before tabs\n" . $herevet) &&
 			    $fix) {
 				while ($fixed[$fixlinenr] =~
-					   s/(^\+.*) {8,8}\t/$1\t\t/) {}
+					   s/(^\+.*) {$tabsize,$tabsize}\t/$1\t\t/) {}
 				while ($fixed[$fixlinenr] =~
 					   s/(^\+.*) +\t/$1\t/) {}
 			}
@@ -3194,11 +3197,11 @@ sub process {
 		if ($perl_version_ok &&
 		    $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) {
 			my $indent = length($1);
-			if ($indent % 8) {
+			if ($indent % $tabsize) {
 				if (WARN("TABSTOP",
 					 "Statements should start on a tabstop\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
+					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/$tabsize)@e;
 				}
 			}
 		}
@@ -3216,8 +3219,8 @@ sub process {
 				my $newindent = $2;
 
 				my $goodtabindent = $oldindent .
-					"\t" x ($pos / 8) .
-					" "  x ($pos % 8);
+					"\t" x ($pos / $tabsize) .
+					" "  x ($pos % $tabsize);
 				my $goodspaceindent = $oldindent . " "  x $pos;
 
 				if ($newindent ne $goodtabindent &&
@@ -3688,11 +3691,11 @@ sub process {
 			#print "line<$line> prevline<$prevline> indent<$indent> sindent<$sindent> check<$check> continuation<$continuation> s<$s> cond_lines<$cond_lines> stat_real<$stat_real> stat<$stat>\n";
 
 			if ($check && $s ne '' &&
-			    (($sindent % 8) != 0 ||
+			    (($sindent % $tabsize) != 0 ||
 			     ($sindent < $indent) ||
 			     ($sindent == $indent &&
 			      ($s !~ /^\s*(?:\}|\{|else\b)/)) ||
-			     ($sindent > $indent + 8))) {
+			     ($sindent > $indent + $tabsize))) {
 				WARN("SUSPECT_CODE_INDENT",
 				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* RE: [PATCH v2] checkpatch: add command-line option for TAB size
  2019-05-08 17:43 [PATCH v2] checkpatch: add command-line option for TAB size Antonio Borneo
  2019-05-08 17:56 ` Joe Perches
@ 2019-05-08 21:14 ` Elliott, Robert (Servers)
  2019-05-08 21:59   ` Joe Perches
  2019-05-08 22:03   ` Antonio Borneo
  1 sibling, 2 replies; 27+ messages in thread
From: Elliott, Robert (Servers) @ 2019-05-08 21:14 UTC (permalink / raw)
  To: Antonio Borneo, Joe Perches, Andy Whitcroft; +Cc: linux-kernel



> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of
> Antonio Borneo
> Sent: Wednesday, May 8, 2019 12:44 PM
> Subject: [PATCH v2] checkpatch: add command-line option for TAB size
...
> Add a command-line option "--tab-size" to let the user select a
> TAB size value other than 8.
> This makes easy to reuse this script by other projects with
> different requirements in their coding style (e.g. OpenOCD [1]
> requires TAB size of 4 characters [2]).
...
> +  --tab-size=n               set the number of spaces for tab (default 8)
...
> +	'tab-size=i'	=> \$tabsize,
...
> -			for (; ($n % 8) != 0; $n++) {
> +			for (; ($n % $tabsize) != 0; $n++) {
...
> -			if ($indent % 8) {
> +			if ($indent % $tabsize) {
> -					"\t" x ($pos / 8) .
> -					" "  x ($pos % 8);
> +					"\t" x ($pos / $tabsize) .
> +					" "  x ($pos % $tabsize);
...
> -			    (($sindent % 8) != 0 ||
> +			    (($sindent % $tabsize) != 0 ||
...
> -			     ($sindent > $indent + 8))) {
> +			     ($sindent > $indent + $tabsize))) {

Checking for 0 before using the value in division and modulo
operations would be prudent.


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] checkpatch: add command-line option for TAB size
  2019-05-08 21:14 ` [PATCH v2] " Elliott, Robert (Servers)
@ 2019-05-08 21:59   ` Joe Perches
  2019-05-08 22:03   ` Antonio Borneo
  1 sibling, 0 replies; 27+ messages in thread
From: Joe Perches @ 2019-05-08 21:59 UTC (permalink / raw)
  To: Elliott, Robert (Servers), Antonio Borneo, Andy Whitcroft; +Cc: linux-kernel

On Wed, 2019-05-08 at 21:14 +0000, Elliott, Robert (Servers) wrote:
> > -----Original Message-----
> > From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-owner@vger.kernel.org] On Behalf Of
> > Antonio Borneo
> > Sent: Wednesday, May 8, 2019 12:44 PM
> > Subject: [PATCH v2] checkpatch: add command-line option for TAB size
> ...
> > Add a command-line option "--tab-size" to let the user select a
> > TAB size value other than 8.
> > This makes easy to reuse this script by other projects with
> > different requirements in their coding style (e.g. OpenOCD [1]
> > requires TAB size of 4 characters [2]).
> ...
> > +  --tab-size=n               set the number of spaces for tab (default 8)
> ...
> > +	'tab-size=i'	=> \$tabsize,
> ...
> > -			for (; ($n % 8) != 0; $n++) {
> > +			for (; ($n % $tabsize) != 0; $n++) {
> ...
> > -			if ($indent % 8) {
> > +			if ($indent % $tabsize) {
> > -					"\t" x ($pos / 8) .
> > -					" "  x ($pos % 8);
> > +					"\t" x ($pos / $tabsize) .
> > +					" "  x ($pos % $tabsize);
> ...
> > -			    (($sindent % 8) != 0 ||
> > +			    (($sindent % $tabsize) != 0 ||
> ...
> > -			     ($sindent > $indent + 8))) {
> > +			     ($sindent > $indent + $tabsize))) {
> 
> Checking for 0 before using the value in division and modulo
> operations would be prudent.

true.

Maybe test $tabsize for <= 0 after GetOptions and error out
if so.



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] checkpatch: add command-line option for TAB size
  2019-05-08 21:14 ` [PATCH v2] " Elliott, Robert (Servers)
  2019-05-08 21:59   ` Joe Perches
@ 2019-05-08 22:03   ` Antonio Borneo
  1 sibling, 0 replies; 27+ messages in thread
From: Antonio Borneo @ 2019-05-08 22:03 UTC (permalink / raw)
  To: Elliott, Robert (Servers); +Cc: Joe Perches, Andy Whitcroft, linux-kernel

On Wed, May 8, 2019 at 11:14 PM Elliott, Robert (Servers)
<elliott@hpe.com> wrote:
...
> Checking for 0 before using the value in division and modulo
> operations would be prudent.

True!
From command line, $tabsize is parsed as integer so I should sort out
any non-positive value.
Will add a check after GetOptions(...)
die "Invalid TAB size: $tabsize\n" if ($tabsize <= 0);

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v4] checkpatch: add command-line option for TAB size
@ 2019-05-09  7:21 Antonio Borneo
  2019-05-09  8:03 ` Joe Perches
  0 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-09  7:21 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft, Elliott, Robert (Servers); +Cc: linux-kernel

Linux kernel coding style requires a size of 8 characters for
both TAB and indentation, and such value is embedded as magic
value allover the checkpatch script.
This makes hard to reuse the script by other projects with
different requirements in their coding style (e.g. OpenOCD [1]
requires TAB size of 4 characters [2]).

Replace the magic value 8 with a variable.

Add a command-line option "--tab-size" to let the user select a
TAB size value other than 8.

[1] http://openocd.org/
[2] http://openocd.org/doc/doxygen/html/stylec.html#styleformat

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Signed-off-by: Erik Ahlén <erik.ahlen@avalonenterprise.com>
Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>
---
v1 -> v2
	add the command line option

v2 -> v3
	rewrite commit msg to remove script readability issue

v3 -> v4
	check for command line value positive
---
 scripts/checkpatch.pl | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 916a3fbd4d47..216f2c8db7aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -62,6 +62,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
+my $tabsize = 8;
 
 sub help {
 	my ($exitcode) = @_;
@@ -96,6 +97,7 @@ Options:
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
   --min-conf-desc-length=n   set the min description length, if shorter, warn
+  --tab-size=n               set the number of spaces for tab (default 8)
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
   --mailback                 only produce a report in case of warnings/errors
@@ -213,6 +215,7 @@ GetOptions(
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
+	'tab-size=i'	=> \$tabsize,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
@@ -265,6 +268,8 @@ if ($color =~ /^[01]$/) {
 	die "Invalid color mode: $color\n";
 }
 
+die "Invalid TAB size: $tabsize\n" if ($tabsize <= 0);
+
 sub hash_save_array_words {
 	my ($hashRef, $arrayRef) = @_;
 
@@ -1211,7 +1216,7 @@ sub expand_tabs {
 		if ($c eq "\t") {
 			$res .= ' ';
 			$n++;
-			for (; ($n % 8) != 0; $n++) {
+			for (; ($n % $tabsize) != 0; $n++) {
 				$res .= ' ';
 			}
 			next;
@@ -2224,7 +2229,7 @@ sub string_find_replace {
 sub tabify {
 	my ($leading) = @_;
 
-	my $source_indent = 8;
+	my $source_indent = $tabsize;
 	my $max_spaces_before_tab = $source_indent - 1;
 	my $spaces_to_tab = " " x $source_indent;
 
@@ -3153,7 +3158,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
-# more than 8 must use tabs.
+# more than $tabsize must use tabs.
 		if ($rawline =~ /^\+\s* \t\s*\S/ ||
 		    $rawline =~ /^\+\s*        \s*/) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
@@ -3172,7 +3177,7 @@ sub process {
 				"please, no space before tabs\n" . $herevet) &&
 			    $fix) {
 				while ($fixed[$fixlinenr] =~
-					   s/(^\+.*) {8,8}\t/$1\t\t/) {}
+					   s/(^\+.*) {$tabsize,$tabsize}\t/$1\t\t/) {}
 				while ($fixed[$fixlinenr] =~
 					   s/(^\+.*) +\t/$1\t/) {}
 			}
@@ -3194,11 +3199,11 @@ sub process {
 		if ($perl_version_ok &&
 		    $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) {
 			my $indent = length($1);
-			if ($indent % 8) {
+			if ($indent % $tabsize) {
 				if (WARN("TABSTOP",
 					 "Statements should start on a tabstop\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
+					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/$tabsize)@e;
 				}
 			}
 		}
@@ -3216,8 +3221,8 @@ sub process {
 				my $newindent = $2;
 
 				my $goodtabindent = $oldindent .
-					"\t" x ($pos / 8) .
-					" "  x ($pos % 8);
+					"\t" x ($pos / $tabsize) .
+					" "  x ($pos % $tabsize);
 				my $goodspaceindent = $oldindent . " "  x $pos;
 
 				if ($newindent ne $goodtabindent &&
@@ -3688,11 +3693,11 @@ sub process {
 			#print "line<$line> prevline<$prevline> indent<$indent> sindent<$sindent> check<$check> continuation<$continuation> s<$s> cond_lines<$cond_lines> stat_real<$stat_real> stat<$stat>\n";
 
 			if ($check && $s ne '' &&
-			    (($sindent % 8) != 0 ||
+			    (($sindent % $tabsize) != 0 ||
 			     ($sindent < $indent) ||
 			     ($sindent == $indent &&
 			      ($s !~ /^\s*(?:\}|\{|else\b)/)) ||
-			     ($sindent > $indent + 8))) {
+			     ($sindent > $indent + $tabsize))) {
 				WARN("SUSPECT_CODE_INDENT",
 				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4] checkpatch: add command-line option for TAB size
  2019-05-09  7:21 [PATCH v4] " Antonio Borneo
@ 2019-05-09  8:03 ` Joe Perches
  2019-05-09  9:26   ` Antonio Borneo
  0 siblings, 1 reply; 27+ messages in thread
From: Joe Perches @ 2019-05-09  8:03 UTC (permalink / raw)
  To: Antonio Borneo, Andy Whitcroft, Elliott, Robert (Servers); +Cc: linux-kernel

On Thu, 2019-05-09 at 09:21 +0200, Antonio Borneo wrote:
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2224,7 +2229,7 @@ sub string_find_replace {
>  sub tabify {
>  	my ($leading) = @_;
>  
> -	my $source_indent = 8;
> +	my $source_indent = $tabsize;
>  	my $max_spaces_before_tab = $source_indent - 1;

I didn't test this.

Does this work properly if --tab-size=1 ?
Maybe die if ($tabsize < 2); is necessary?



^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v4] checkpatch: add command-line option for TAB size
  2019-05-09  8:03 ` Joe Perches
@ 2019-05-09  9:26   ` Antonio Borneo
  2019-05-09  9:39     ` [PATCH v5] " Antonio Borneo
  0 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-09  9:26 UTC (permalink / raw)
  To: Joe Perches; +Cc: Andy Whitcroft, Elliott, Robert (Servers), linux-kernel

On Thu, May 9, 2019 at 10:03 AM Joe Perches <joe@perches.com> wrote:
>
> On Thu, 2019-05-09 at 09:21 +0200, Antonio Borneo wrote:
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2224,7 +2229,7 @@ sub string_find_replace {
> >  sub tabify {
> >       my ($leading) = @_;
> >
> > -     my $source_indent = 8;
> > +     my $source_indent = $tabsize;
> >       my $max_spaces_before_tab = $source_indent - 1;
>
> I didn't test this.
>
> Does this work properly if --tab-size=1 ?
> Maybe die if ($tabsize < 2); is necessary?

I have tested it and it works fine, but now that you point it, I
rechecked the code. There is already this in checkpatch
sub tabify {
...
        my $source_indent = $tabsize;
        my $max_spaces_before_tab = $source_indent - 1;
...
        #Remove spaces before a tab
        1 while $leading =~ s@^([\t]*)( {1,$max_spaces_before_tab})\t@$1\t@g;
that works fine.
But we could have in the future some other test in checkpatch that
uses "$tabsize - 1" and does not get the proper validation for
--tab-size=1
Maybe it's safer to put die if ($tabsize < 2) right now, and avoid
future headaches. With a comment to explains this choice.

Antonio

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH v5] checkpatch: add command-line option for TAB size
@ 2019-05-09  9:39     ` Antonio Borneo
  2020-01-22 16:38       ` [PATCH V6] " Antonio Borneo
  0 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2019-05-09  9:39 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft, Elliott, Robert (Servers); +Cc: linux-kernel

Linux kernel coding style requires a size of 8 characters for
both TAB and indentation, and such value is embedded as magic
value allover the checkpatch script.
This makes hard to reuse the script by other projects with
different requirements in their coding style (e.g. OpenOCD [1]
requires TAB size of 4 characters [2]).

Replace the magic value 8 with a variable.

Add a command-line option "--tab-size" to let the user select a
TAB size value other than 8.

[1] http://openocd.org/
[2] http://openocd.org/doc/doxygen/html/stylec.html#styleformat

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Signed-off-by: Erik Ahlén <erik.ahlen@avalonenterprise.com>
Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>
---
v1 -> v2
	add the command line option

v2 -> v3
	rewrite commit msg to remove script readability issue

v3 -> v4
	check for command line value positive

v4 -> v5
	exclude --tab-size=1
---
 scripts/checkpatch.pl | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 916a3fbd4d47..c36876f78bd5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -62,6 +62,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
 my $typedefsfile = "";
 my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
+my $tabsize = 8;
 
 sub help {
 	my ($exitcode) = @_;
@@ -96,6 +97,7 @@ Options:
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
   --min-conf-desc-length=n   set the min description length, if shorter, warn
+  --tab-size=n               set the number of spaces for tab (default 8)
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
   --mailback                 only produce a report in case of warnings/errors
@@ -213,6 +215,7 @@ GetOptions(
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
+	'tab-size=i'	=> \$tabsize,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
@@ -265,6 +268,9 @@ if ($color =~ /^[01]$/) {
 	die "Invalid color mode: $color\n";
 }
 
+# skip TAB size 1 to avoid additional checks on $tabsize - 1
+die "Invalid TAB size: $tabsize\n" if ($tabsize < 2);
+
 sub hash_save_array_words {
 	my ($hashRef, $arrayRef) = @_;
 
@@ -1211,7 +1217,7 @@ sub expand_tabs {
 		if ($c eq "\t") {
 			$res .= ' ';
 			$n++;
-			for (; ($n % 8) != 0; $n++) {
+			for (; ($n % $tabsize) != 0; $n++) {
 				$res .= ' ';
 			}
 			next;
@@ -2224,7 +2230,7 @@ sub string_find_replace {
 sub tabify {
 	my ($leading) = @_;
 
-	my $source_indent = 8;
+	my $source_indent = $tabsize;
 	my $max_spaces_before_tab = $source_indent - 1;
 	my $spaces_to_tab = " " x $source_indent;
 
@@ -3153,7 +3159,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
-# more than 8 must use tabs.
+# more than $tabsize must use tabs.
 		if ($rawline =~ /^\+\s* \t\s*\S/ ||
 		    $rawline =~ /^\+\s*        \s*/) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
@@ -3172,7 +3178,7 @@ sub process {
 				"please, no space before tabs\n" . $herevet) &&
 			    $fix) {
 				while ($fixed[$fixlinenr] =~
-					   s/(^\+.*) {8,8}\t/$1\t\t/) {}
+					   s/(^\+.*) {$tabsize,$tabsize}\t/$1\t\t/) {}
 				while ($fixed[$fixlinenr] =~
 					   s/(^\+.*) +\t/$1\t/) {}
 			}
@@ -3194,11 +3200,11 @@ sub process {
 		if ($perl_version_ok &&
 		    $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) {
 			my $indent = length($1);
-			if ($indent % 8) {
+			if ($indent % $tabsize) {
 				if (WARN("TABSTOP",
 					 "Statements should start on a tabstop\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
+					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/$tabsize)@e;
 				}
 			}
 		}
@@ -3216,8 +3222,8 @@ sub process {
 				my $newindent = $2;
 
 				my $goodtabindent = $oldindent .
-					"\t" x ($pos / 8) .
-					" "  x ($pos % 8);
+					"\t" x ($pos / $tabsize) .
+					" "  x ($pos % $tabsize);
 				my $goodspaceindent = $oldindent . " "  x $pos;
 
 				if ($newindent ne $goodtabindent &&
@@ -3688,11 +3694,11 @@ sub process {
 			#print "line<$line> prevline<$prevline> indent<$indent> sindent<$sindent> check<$check> continuation<$continuation> s<$s> cond_lines<$cond_lines> stat_real<$stat_real> stat<$stat>\n";
 
 			if ($check && $s ne '' &&
-			    (($sindent % 8) != 0 ||
+			    (($sindent % $tabsize) != 0 ||
 			     ($sindent < $indent) ||
 			     ($sindent == $indent &&
 			      ($s !~ /^\s*(?:\}|\{|else\b)/)) ||
-			     ($sindent > $indent + 8))) {
+			     ($sindent > $indent + $tabsize))) {
 				WARN("SUSPECT_CODE_INDENT",
 				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
-- 
2.21.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH v2] checkpatch: fix multiple const * types
  2019-05-08 17:43   ` [PATCH v2] " Antonio Borneo
@ 2019-09-04  9:37     ` Antonio Borneo
  0 siblings, 0 replies; 27+ messages in thread
From: Antonio Borneo @ 2019-09-04  9:37 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: linux-kernel

Hi Joe,

can this series be queued for merge ?

I just found I mess-up the mail thread references, so here are the
latest versions:

[1/4 V2] https://lore.kernel.org/lkml/20190508174347.13901-1-borneo.antonio@gmail.com/
[2/4] dropped
[3/4] https://lore.kernel.org/lkml/20190508122721.7513-3-borneo.antonio@gmail.com/
[4/4 V5] https://lore.kernel.org/lkml/20190508122721.7513-3-borneo.antonio@gmail.com/

Thanks,
Antonio

^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V3] checkpatch: fix multiple const * types
  2019-05-08 15:26   ` Antonio Borneo
@ 2020-01-22 16:38     ` Antonio Borneo
  0 siblings, 0 replies; 27+ messages in thread
From: Antonio Borneo @ 2020-01-22 16:38 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: Antonio Borneo, linux-kernel

Commit 1574a29f8e76 ("checkpatch: allow multiple const * types")
claims to support repetition of pattern "const *", but it actually
allows only one extra instance.
Check the following lines
	int a(char const * const x[]);
	int b(char const * const *x);
	int c(char const * const * const x[]);
	int d(char const * const * const *x);
with command
	./scripts/checkpatch.pl --show-types -f filename
to find that only the first line passes the test, while a warning
is triggered by the other 3 lines:
	WARNING:FUNCTION_ARGUMENTS: function definition argument
	'char const * const' should also have an identifier name
The reason is that the pattern match halts at the second asterisk
in the line, thus the remaining text starting with asterisk fails
to match a valid name for a variable.

Fixed by replacing "?" (Match 1 or 0 times) with "{0,4}" (Match
no more than 4 times) in the regular expression.
Fix also the similar test for types in unusual order.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Fixes: 1574a29f8e76 ("checkpatch: allow multiple const * types")

---

V1->V2
	use a max match {0,4} instead of *, to limit the memory
	used by perl

V2->V3
	rebased

---
 scripts/checkpatch.pl | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a63380c6b0d2..30da08d9646a 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -804,12 +804,12 @@ sub build_types {
 		  }x;
 	$Type	= qr{
 			$NonptrType
-			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)?
+			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+){0,4}
 			(?:\s+$Inline|\s+$Modifier)*
 		  }x;
 	$TypeMisordered	= qr{
 			$NonptrTypeMisordered
-			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+)?
+			(?:(?:\s|\*|\[\])+\s*const|(?:\s|\*\s*(?:const\s*)?|\[\])+|(?:\s*\[\s*\])+){0,4}
 			(?:\s+$Inline|\s+$Modifier)*
 		  }x;
 	$Declare	= qr{(?:$Storage\s+(?:$Inline\s+)?)?$Type};
-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V2] checkpatch: fix minor typo and mixed space+tab in indentation
  2019-05-08 12:27 ` [PATCH 3/4] checkpatch: fix minor typo and mixed space+tab in indentation Antonio Borneo
@ 2020-01-22 16:38   ` Antonio Borneo
  2020-01-22 17:26     ` Joe Perches
  0 siblings, 1 reply; 27+ messages in thread
From: Antonio Borneo @ 2020-01-22 16:38 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: Antonio Borneo, linux-kernel

Fix spelling of "concatenation".
Don't use tab after space in indentation.

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>

---

v1 -> v2
	rebased

---
 scripts/checkpatch.pl | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 30da08d9646a..4c1be774b0ed 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4582,7 +4582,7 @@ sub process {
 					    ($op eq '>' &&
 					     $ca =~ /<\S+\@\S+$/))
 					{
-					    	$ok = 1;
+						$ok = 1;
 					}
 
 					# for asm volatile statements
@@ -4917,7 +4917,7 @@ sub process {
 			# conditional.
 			substr($s, 0, length($c), '');
 			$s =~ s/\n.*//g;
-			$s =~ s/$;//g; 	# Remove any comments
+			$s =~ s/$;//g;	# Remove any comments
 			if (length($c) && $s !~ /^\s*{?\s*\\*\s*$/ &&
 			    $c !~ /}\s*while\s*/)
 			{
@@ -4956,7 +4956,7 @@ sub process {
 # if and else should not have general statements after it
 		if ($line =~ /^.\s*(?:}\s*)?else\b(.*)/) {
 			my $s = $1;
-			$s =~ s/$;//g; 	# Remove any comments
+			$s =~ s/$;//g;	# Remove any comments
 			if ($s !~ /^\s*(?:\sif|(?:{|)\s*\\?\s*$)/) {
 				ERROR("TRAILING_STATEMENTS",
 				      "trailing statements should be on next line\n" . $herecurr);
@@ -5132,7 +5132,7 @@ sub process {
 			{
 			}
 
-			# Flatten any obvious string concatentation.
+			# Flatten any obvious string concatenation.
 			while ($dstat =~ s/($String)\s*$Ident/$1/ ||
 			       $dstat =~ s/$Ident\s*($String)/$1/)
 			{
-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* [PATCH V6] checkpatch: add command-line option for TAB size
  2019-05-09  9:39     ` [PATCH v5] " Antonio Borneo
@ 2020-01-22 16:38       ` Antonio Borneo
  0 siblings, 0 replies; 27+ messages in thread
From: Antonio Borneo @ 2020-01-22 16:38 UTC (permalink / raw)
  To: Joe Perches, Andy Whitcroft; +Cc: Antonio Borneo, linux-kernel

Linux kernel coding style requires a size of 8 characters for
both TAB and indentation, and such value is embedded as magic
value allover the checkpatch script.
This makes hard to reuse the script by other projects with
different requirements in their coding style (e.g. OpenOCD [1]
requires TAB size of 4 characters [2]).

Replace the magic value 8 with a variable.

Add a command-line option "--tab-size" to let the user select a
TAB size value other than 8.

[1] http://openocd.org/
[2] http://openocd.org/doc/doxygen/html/stylec.html#styleformat

Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
Signed-off-by: Erik Ahlén <erik.ahlen@avalonenterprise.com>
Signed-off-by: Spencer Oliver <spen@spen-soft.co.uk>

---

v1 -> v2
        add the command line option

v2 -> v3
        rewrite commit msg to remove script readability issue

v3 -> v4
        check for command line value positive

v4 -> v5
        exclude --tab-size=1

v5 -> v6
	rebased

---
 scripts/checkpatch.pl | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4c1be774b0ed..8bac825665ef 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -64,6 +64,7 @@ my $color = "auto";
 my $allow_c99_comments = 1; # Can be overridden by --ignore C99_COMMENT_TOLERANCE
 # git output parsing needs US English output, so first set backtick child process LANGUAGE
 my $git_command ='export LANGUAGE=en_US.UTF-8; git';
+my $tabsize = 8;
 
 sub help {
 	my ($exitcode) = @_;
@@ -98,6 +99,7 @@ Options:
   --show-types               show the specific message type in the output
   --max-line-length=n        set the maximum line length, if exceeded, warn
   --min-conf-desc-length=n   set the min description length, if shorter, warn
+  --tab-size=n               set the number of spaces for tab (default 8)
   --root=PATH                PATH to the kernel tree root
   --no-summary               suppress the per-file summary
   --mailback                 only produce a report in case of warnings/errors
@@ -215,6 +217,7 @@ GetOptions(
 	'list-types!'	=> \$list_types,
 	'max-line-length=i' => \$max_line_length,
 	'min-conf-desc-length=i' => \$min_conf_desc_length,
+	'tab-size=i'	=> \$tabsize,
 	'root=s'	=> \$root,
 	'summary!'	=> \$summary,
 	'mailback!'	=> \$mailback,
@@ -267,6 +270,9 @@ if ($color =~ /^[01]$/) {
 	die "Invalid color mode: $color\n";
 }
 
+# skip TAB size 1 to avoid additional checks on $tabsize - 1
+die "Invalid TAB size: $tabsize\n" if ($tabsize < 2);
+
 sub hash_save_array_words {
 	my ($hashRef, $arrayRef) = @_;
 
@@ -1217,7 +1223,7 @@ sub expand_tabs {
 		if ($c eq "\t") {
 			$res .= ' ';
 			$n++;
-			for (; ($n % 8) != 0; $n++) {
+			for (; ($n % $tabsize) != 0; $n++) {
 				$res .= ' ';
 			}
 			next;
@@ -2230,7 +2236,7 @@ sub string_find_replace {
 sub tabify {
 	my ($leading) = @_;
 
-	my $source_indent = 8;
+	my $source_indent = $tabsize;
 	my $max_spaces_before_tab = $source_indent - 1;
 	my $spaces_to_tab = " " x $source_indent;
 
@@ -3198,7 +3204,7 @@ sub process {
 		next if ($realfile !~ /\.(h|c|pl|dtsi|dts)$/);
 
 # at the beginning of a line any tabs must come first and anything
-# more than 8 must use tabs.
+# more than $tabsize must use tabs.
 		if ($rawline =~ /^\+\s* \t\s*\S/ ||
 		    $rawline =~ /^\+\s*        \s*/) {
 			my $herevet = "$here\n" . cat_vet($rawline) . "\n";
@@ -3217,7 +3223,7 @@ sub process {
 				"please, no space before tabs\n" . $herevet) &&
 			    $fix) {
 				while ($fixed[$fixlinenr] =~
-					   s/(^\+.*) {8,8}\t/$1\t\t/) {}
+					   s/(^\+.*) {$tabsize,$tabsize}\t/$1\t\t/) {}
 				while ($fixed[$fixlinenr] =~
 					   s/(^\+.*) +\t/$1\t/) {}
 			}
@@ -3239,11 +3245,11 @@ sub process {
 		if ($perl_version_ok &&
 		    $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$)|$Declare\s*$Ident\s*[;=])/) {
 			my $indent = length($1);
-			if ($indent % 8) {
+			if ($indent % $tabsize) {
 				if (WARN("TABSTOP",
 					 "Statements should start on a tabstop\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
+					$fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/$tabsize)@e;
 				}
 			}
 		}
@@ -3261,8 +3267,8 @@ sub process {
 				my $newindent = $2;
 
 				my $goodtabindent = $oldindent .
-					"\t" x ($pos / 8) .
-					" "  x ($pos % 8);
+					"\t" x ($pos / $tabsize) .
+					" "  x ($pos % $tabsize);
 				my $goodspaceindent = $oldindent . " "  x $pos;
 
 				if ($newindent ne $goodtabindent &&
@@ -3733,11 +3739,11 @@ sub process {
 			#print "line<$line> prevline<$prevline> indent<$indent> sindent<$sindent> check<$check> continuation<$continuation> s<$s> cond_lines<$cond_lines> stat_real<$stat_real> stat<$stat>\n";
 
 			if ($check && $s ne '' &&
-			    (($sindent % 8) != 0 ||
+			    (($sindent % $tabsize) != 0 ||
 			     ($sindent < $indent) ||
 			     ($sindent == $indent &&
 			      ($s !~ /^\s*(?:\}|\{|else\b)/)) ||
-			     ($sindent > $indent + 8))) {
+			     ($sindent > $indent + $tabsize))) {
 				WARN("SUSPECT_CODE_INDENT",
 				     "suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
-- 
2.25.0


^ permalink raw reply	[flat|nested] 27+ messages in thread

* Re: [PATCH V2] checkpatch: fix minor typo and mixed space+tab in indentation
  2020-01-22 16:38   ` [PATCH V2] " Antonio Borneo
@ 2020-01-22 17:26     ` Joe Perches
  0 siblings, 0 replies; 27+ messages in thread
From: Joe Perches @ 2020-01-22 17:26 UTC (permalink / raw)
  To: Antonio Borneo, Andy Whitcroft, Andrew Morton; +Cc: linux-kernel

On Wed, 2020-01-22 at 17:38 +0100, Antonio Borneo wrote:
> Fix spelling of "concatenation".
> Don't use tab after space in indentation.
> 
> Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>

I've no objection to any of these 3 patches.
Andrew, might you pick them up please?

https://lore.kernel.org/patchwork/patch/1183806/
https://lore.kernel.org/patchwork/patch/1183805/
https://lore.kernel.org/patchwork/patch/1183804/

> ---
> 
> v1 -> v2
> 	rebased
> 
> ---
>  scripts/checkpatch.pl | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 30da08d9646a..4c1be774b0ed 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4582,7 +4582,7 @@ sub process {
>  					    ($op eq '>' &&
>  					     $ca =~ /<\S+\@\S+$/))
>  					{
> -					    	$ok = 1;
> +						$ok = 1;
>  					}
>  
>  					# for asm volatile statements
> @@ -4917,7 +4917,7 @@ sub process {
>  			# conditional.
>  			substr($s, 0, length($c), '');
>  			$s =~ s/\n.*//g;
> -			$s =~ s/$;//g; 	# Remove any comments
> +			$s =~ s/$;//g;	# Remove any comments
>  			if (length($c) && $s !~ /^\s*{?\s*\\*\s*$/ &&
>  			    $c !~ /}\s*while\s*/)
>  			{
> @@ -4956,7 +4956,7 @@ sub process {
>  # if and else should not have general statements after it
>  		if ($line =~ /^.\s*(?:}\s*)?else\b(.*)/) {
>  			my $s = $1;
> -			$s =~ s/$;//g; 	# Remove any comments
> +			$s =~ s/$;//g;	# Remove any comments
>  			if ($s !~ /^\s*(?:\sif|(?:{|)\s*\\?\s*$)/) {
>  				ERROR("TRAILING_STATEMENTS",
>  				      "trailing statements should be on next line\n" . $herecurr);
> @@ -5132,7 +5132,7 @@ sub process {
>  			{
>  			}
>  
> -			# Flatten any obvious string concatentation.
> +			# Flatten any obvious string concatenation.
>  			while ($dstat =~ s/($String)\s*$Ident/$1/ ||
>  			       $dstat =~ s/$Ident\s*($String)/$1/)
>  			{


^ permalink raw reply	[flat|nested] 27+ messages in thread

end of thread, other threads:[~2020-01-22 17:28 UTC | newest]

Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-08 12:27 [PATCH 1/4] checkpatch: fix multiple const * types Antonio Borneo
2019-05-08 12:27 ` [PATCH 2/4] checkpatch: add --fix for warning LINE_CONTINUATIONS Antonio Borneo
2019-05-08 14:19   ` Joe Perches
2019-05-08 12:27 ` [PATCH 3/4] checkpatch: fix minor typo and mixed space+tab in indentation Antonio Borneo
2020-01-22 16:38   ` [PATCH V2] " Antonio Borneo
2020-01-22 17:26     ` Joe Perches
2019-05-08 12:27 ` [PATCH 4/4] checkpatch: replace magic value for TAB size Antonio Borneo
2019-05-08 14:52   ` Joe Perches
2019-05-08 15:32     ` Antonio Borneo
2019-05-08 17:02       ` Joe Perches
2019-05-08 14:51 ` [PATCH 1/4] checkpatch: fix multiple const * types Joe Perches
2019-05-08 15:26   ` Antonio Borneo
2020-01-22 16:38     ` [PATCH V3] " Antonio Borneo
2019-05-08 17:43   ` [PATCH v2] " Antonio Borneo
2019-09-04  9:37     ` Antonio Borneo
2019-05-08 17:43 [PATCH v2] checkpatch: add command-line option for TAB size Antonio Borneo
2019-05-08 17:56 ` Joe Perches
2019-05-08 18:19   ` Antonio Borneo
2019-05-08 18:36     ` [PATCH v3] " Antonio Borneo
2019-05-08 21:14 ` [PATCH v2] " Elliott, Robert (Servers)
2019-05-08 21:59   ` Joe Perches
2019-05-08 22:03   ` Antonio Borneo
2019-05-09  7:21 [PATCH v4] " Antonio Borneo
2019-05-09  8:03 ` Joe Perches
2019-05-09  9:26   ` Antonio Borneo
2019-05-09  9:39     ` [PATCH v5] " Antonio Borneo
2020-01-22 16:38       ` [PATCH V6] " Antonio Borneo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).