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 <[email protected]>
Signed-off-by: Erik Ahlén <[email protected]>
Signed-off-by: Spencer Oliver <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Erik Ahl?n <[email protected]>
> Signed-off-by: Spencer Oliver <[email protected]>
> ---
> 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");
> }
On Wed, May 8, 2019 at 7:56 PM Joe Perches <[email protected]> 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
> -----Original Message-----
> From: [email protected] [mailto:[email protected]] 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.
On Wed, 2019-05-08 at 21:14 +0000, Elliott, Robert (Servers) wrote:
> > -----Original Message-----
> > From: [email protected] [mailto:[email protected]] 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.
On Wed, May 8, 2019 at 11:14 PM Elliott, Robert (Servers)
<[email protected]> 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);