* [PATCH v2] checkpatch: add several Kconfig default value tests
@ 2019-07-04 9:40 ` Miles Chen
0 siblings, 0 replies; 5+ messages in thread
From: Miles Chen @ 2019-07-04 9:40 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches
Cc: linux-kernel, linux-mediatek, wsd_upstream, Miles Chen, Yingjoe Chen
This change adds 3 Kconfig default value tests:
1. discourage default n cases:
e.g.,
default n
2. discourage default "[ynm]" cases:
e.g.,
arch/powerpc/Kconfig: default "y" if PPC_POWERNV
arch/powerpc/Kconfig: default "y" if PPC_POWERNV
arch/powerpc/Kconfig: default "n"
drivers/auxdisplay/Kconfig: default "n"
drivers/crypto/Kconfig: default "m"
drivers/rapidio/devices/Kconfig: default "n"
3. discourage default \!?EXPERT cases:
e.g.,
drivers/hid/Kconfig: default !EXPERT
tested cases:
default m
default n if ALPHA_EV5 || ALPHA_EV56 || (ALPHA_EV4 && !ALPHA_LCA)
default y if ALPHA_QEMU
default n if PPC_POWERNV
default n
default EXPERT
default !EXPERT
default "m"
default "n"
default "y" if EXPERT
default "y" if PPC_POWERNV
test result:
WARNING: 'default n' is the default value, no need to write it explicitly.
+ default n
WARNING: Avoid default turn on kernel configs by default !?EXPERT
+ default EXPERT
WARNING: Avoid default turn on kernel configs by default !?EXPERT
+ default !EXPERT
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "m"
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "n"
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "y" if EXPERT
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "y" if PPC_POWERNV
Change since v1:
discourage default n$
discourage default "[ynm]"
discourage default \!?EXPERT
Cc: Joe Perches <joe@perches.com>
Cc: Yingjoe Chen <yingjoe.chen@mediatek.com>
Signed-off-by: Miles Chen <miles.chen@mediatek.com>
---
scripts/checkpatch.pl | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 342c7c781ba5..c1de50202a18 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3005,6 +3005,27 @@ sub process {
"Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
}
+# discourage redundant 'default n'
+ if ($realfile =~ /Kconfig/ &&
+ $line =~ /^\+\s*default n$/) {
+ WARN("DEFAULT_VALUE_STYLE",
+ "'default n' is the default value, no need to write it explicitly.\n" . $herecurr);
+ }
+
+# discourage quote: use default [ynm], not default "[ynm]"
+ if ($realfile =~ /Kconfig/ &&
+ $rawline =~ /^\+\s*default\s*"[ynm]"/) {
+ WARN("DEFAULT_VALUE_STYLE",
+ "Use default [ynm] instead of default \"[ynm]\"\n" . $herecurr);
+ }
+
+# discourage default \!?EXPERT
+ if ($realfile =~ /Kconfig/ &&
+ $line =~ /^\+\s*default \!?EXPERT/) {
+ WARN("DEFAULT_VALUE_STYLE",
+ "Avoid default turn on kernel configs by default !?EXPERT\n" . $herecurr);
+ }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
--
2.18.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2] checkpatch: add several Kconfig default value tests
@ 2019-07-04 9:40 ` Miles Chen
0 siblings, 0 replies; 5+ messages in thread
From: Miles Chen @ 2019-07-04 9:40 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches
Cc: linux-kernel, linux-mediatek, wsd_upstream, Miles Chen, Yingjoe Chen
This change adds 3 Kconfig default value tests:
1. discourage default n cases:
e.g.,
default n
2. discourage default "[ynm]" cases:
e.g.,
arch/powerpc/Kconfig: default "y" if PPC_POWERNV
arch/powerpc/Kconfig: default "y" if PPC_POWERNV
arch/powerpc/Kconfig: default "n"
drivers/auxdisplay/Kconfig: default "n"
drivers/crypto/Kconfig: default "m"
drivers/rapidio/devices/Kconfig: default "n"
3. discourage default \!?EXPERT cases:
e.g.,
drivers/hid/Kconfig: default !EXPERT
tested cases:
default m
default n if ALPHA_EV5 || ALPHA_EV56 || (ALPHA_EV4 && !ALPHA_LCA)
default y if ALPHA_QEMU
default n if PPC_POWERNV
default n
default EXPERT
default !EXPERT
default "m"
default "n"
default "y" if EXPERT
default "y" if PPC_POWERNV
test result:
WARNING: 'default n' is the default value, no need to write it explicitly.
+ default n
WARNING: Avoid default turn on kernel configs by default !?EXPERT
+ default EXPERT
WARNING: Avoid default turn on kernel configs by default !?EXPERT
+ default !EXPERT
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "m"
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "n"
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "y" if EXPERT
WARNING: Use default [ynm] instead of default "[ynm]"
+ default "y" if PPC_POWERNV
Change since v1:
discourage default n$
discourage default "[ynm]"
discourage default \!?EXPERT
Cc: Joe Perches <joe@perches.com>
Cc: Yingjoe Chen <yingjoe.chen@mediatek.com>
Signed-off-by: Miles Chen <miles.chen@mediatek.com>
---
scripts/checkpatch.pl | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 342c7c781ba5..c1de50202a18 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3005,6 +3005,27 @@ sub process {
"Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
}
+# discourage redundant 'default n'
+ if ($realfile =~ /Kconfig/ &&
+ $line =~ /^\+\s*default n$/) {
+ WARN("DEFAULT_VALUE_STYLE",
+ "'default n' is the default value, no need to write it explicitly.\n" . $herecurr);
+ }
+
+# discourage quote: use default [ynm], not default "[ynm]"
+ if ($realfile =~ /Kconfig/ &&
+ $rawline =~ /^\+\s*default\s*"[ynm]"/) {
+ WARN("DEFAULT_VALUE_STYLE",
+ "Use default [ynm] instead of default \"[ynm]\"\n" . $herecurr);
+ }
+
+# discourage default \!?EXPERT
+ if ($realfile =~ /Kconfig/ &&
+ $line =~ /^\+\s*default \!?EXPERT/) {
+ WARN("DEFAULT_VALUE_STYLE",
+ "Avoid default turn on kernel configs by default !?EXPERT\n" . $herecurr);
+ }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
--
2.18.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] checkpatch: add several Kconfig default value tests
2019-07-04 9:40 ` Miles Chen
(?)
@ 2019-07-04 18:49 ` Joe Perches
2019-07-05 2:06 ` Miles Chen
-1 siblings, 1 reply; 5+ messages in thread
From: Joe Perches @ 2019-07-04 18:49 UTC (permalink / raw)
To: Miles Chen, Andy Whitcroft
Cc: linux-kernel, linux-mediatek, wsd_upstream, Yingjoe Chen
On Thu, 2019-07-04 at 17:40 +0800, Miles Chen wrote:
> This change adds 3 Kconfig default value tests:
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3005,6 +3005,27 @@ sub process {
> "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> }
>
> +# discourage redundant 'default n'
> + if ($realfile =~ /Kconfig/ &&
> + $line =~ /^\+\s*default n$/) {
> + WARN("DEFAULT_VALUE_STYLE",
> + "'default n' is the default value, no need to write it explicitly.\n" . $herecurr);
> + }
> +
> +# discourage quote: use default [ynm], not default "[ynm]"
> + if ($realfile =~ /Kconfig/ &&
> + $rawline =~ /^\+\s*default\s*"[ynm]"/) {
> + WARN("DEFAULT_VALUE_STYLE",
> + "Use default [ynm] instead of default \"[ynm]\"\n" . $herecurr);
> + }
> +
> +# discourage default \!?EXPERT
> + if ($realfile =~ /Kconfig/ &&
> + $line =~ /^\+\s*default \!?EXPERT/) {
> + WARN("DEFAULT_VALUE_STYLE",
> + "Avoid default turn on kernel configs by default !?EXPERT\n" . $herecurr);
> + }
> +
I'd prefer to create a block for all the Kconfig file tests and
avoid multiply determining if the filename includes Kconfig so
the script runs a bit faster.
Also some trivial changes to the added tests with added --fix
capability. Something like:
---
scripts/checkpatch.pl | 139 ++++++++++++++++++++++++++++++--------------------
1 file changed, 85 insertions(+), 54 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6cb99ec62000..4780149a8d30 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2934,60 +2934,98 @@ sub process {
"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
-# Only applies when adding the entry originally, after that we do not have
-# sufficient context to determine whether it is indeed long enough.
- if ($realfile =~ /Kconfig/ &&
- # 'choice' is usually the last thing on the line (though
- # Kconfig supports named choices), so use a word boundary
- # (\b) rather than a whitespace character (\s)
- $line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
- my $length = 0;
- my $cnt = $realcnt;
- my $ln = $linenr + 1;
- my $f;
- my $is_start = 0;
- my $is_end = 0;
- for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
- $f = $lines[$ln - 1];
- $cnt-- if ($lines[$ln - 1] !~ /^-/);
- $is_end = $lines[$ln - 1] =~ /^\+/;
-
- next if ($f =~ /^-/);
- last if (!$file && $f =~ /^\@\@/);
-
- if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
- $is_start = 1;
- } elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
- if ($lines[$ln - 1] =~ "---help---") {
- WARN("CONFIG_DESCRIPTION",
- "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
+# Kconfig tests
+ if ($realfile =~ /Kconfig/) {
+ # check for Kconfig help text having a real description
+ # Only applies when adding the entry originally, after
+ # that we do not have sufficient context to determine
+ # whether it is indeed long enough.
+ # 'choice' is usually the last thing on the line (though
+ # Kconfig supports named choices), so use a word
+ # boundary (\b) rather than a whitespace character (\s)
+ if ($line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
+ my $length = 0;
+ my $cnt = $realcnt;
+ my $ln = $linenr + 1;
+ my $f;
+ my $is_start = 0;
+ my $is_end = 0;
+ for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
+ $f = $lines[$ln - 1];
+ $cnt-- if ($lines[$ln - 1] !~ /^-/);
+ $is_end = $lines[$ln - 1] =~ /^\+/;
+
+ next if ($f =~ /^-/);
+ last if (!$file && $f =~ /^\@\@/);
+
+ if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
+ $is_start = 1;
+ } elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
+ if ($lines[$ln - 1] =~ "---help---") {
+ WARN("CONFIG_DESCRIPTION",
+ "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
+ }
+ $length = -1;
+ }
+
+ $f =~ s/^.//;
+ $f =~ s/#.*//;
+ $f =~ s/^\s+//;
+ next if ($f =~ /^$/);
+
+ # This only checks context lines in the patch
+ # and so hopefully shouldn't trigger false
+ # positives, even though some of these are
+ # common words in help texts
+ if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
+ if|endif|menu|endmenu|source)\b/x) {
+ $is_end = 1;
+ last;
}
- $length = -1;
+ $length++;
+ }
+ if ($is_start && $is_end && $length < $min_conf_desc_length) {
+ WARN("CONFIG_DESCRIPTION",
+ "please write a paragraph that describes the config symbol fully\n" . $herecurr);
}
+ #print "is_start<$is_start> is_end<$is_end> length<$length>\n";
+ }
- $f =~ s/^.//;
- $f =~ s/#.*//;
- $f =~ s/^\s+//;
- next if ($f =~ /^$/);
-
- # This only checks context lines in the patch
- # and so hopefully shouldn't trigger false
- # positives, even though some of these are
- # common words in help texts
- if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
- if|endif|menu|endmenu|source)\b/x) {
- $is_end = 1;
- last;
+# discourage the use of boolean for type definition attributes
+ if ($line =~ /^\+\s*\bboolean\b/) {
+ if (WARN("CONFIG_TYPE_BOOLEAN",
+ "Use of boolean is deprecated, please use bool instead\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/\bboolean\b/bool/;
+ }
+ }
+
+# Kconfig: discourage redundant 'default n'
+ if ($line =~ /^\+\s*default\s+n$/) {
+ if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
+ "'default n' is the default value, no need to write it explicitly\n" . $herecurr) &&
+ $fix) {
+ fix_delete_line($fixlinenr, $rawline);
}
- $length++;
}
- if ($is_start && $is_end && $length < $min_conf_desc_length) {
- WARN("CONFIG_DESCRIPTION",
- "please write a paragraph that describes the config symbol fully\n" . $herecurr);
+
+# Kconfig: discourage quoted defaults: use default [ynm], not default "[ynm]"
+ if ($rawline =~ /^\+\s*default\s+"([ynm])"/) {
+ if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
+ "Use 'default $1' not 'default \"$1\"'\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/\b(default\s+)"(.)"/$1$2/;
+ }
+ }
+
+# Kconfig: discourage using default EXPERT or !EXPERT
+ if ($line =~ /^\+\s*default\s+\!?\s*EXPERT\b/) {
+ WARN("CONFIG_DEFAULT_VALUE_STYLE",
+ "Avoid using default EXPERT\n" . $herecurr);
}
- #print "is_start<$is_start> is_end<$is_end> length<$length>\n";
}
+# End of Kconfig tests
+
# check for MAINTAINERS entries that don't have the right form
if ($realfile =~ /^MAINTAINERS$/ &&
@@ -3000,13 +3038,6 @@ sub process {
}
}
-# discourage the use of boolean for type definition attributes of Kconfig options
- if ($realfile =~ /Kconfig/ &&
- $line =~ /^\+\s*\bboolean\b/) {
- WARN("CONFIG_TYPE_BOOLEAN",
- "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
- }
-
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] checkpatch: add several Kconfig default value tests
2019-07-04 18:49 ` Joe Perches
@ 2019-07-05 2:06 ` Miles Chen
0 siblings, 0 replies; 5+ messages in thread
From: Miles Chen @ 2019-07-05 2:06 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Whitcroft, linux-kernel, linux-mediatek, wsd_upstream, Yingjoe Chen
On Thu, 2019-07-04 at 11:49 -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 17:40 +0800, Miles Chen wrote:
> > This change adds 3 Kconfig default value tests:
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3005,6 +3005,27 @@ sub process {
> > "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> > }
> >
> > +# discourage redundant 'default n'
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*default n$/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "'default n' is the default value, no need to write it explicitly.\n" . $herecurr);
> > + }
> > +
> > +# discourage quote: use default [ynm], not default "[ynm]"
> > + if ($realfile =~ /Kconfig/ &&
> > + $rawline =~ /^\+\s*default\s*"[ynm]"/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "Use default [ynm] instead of default \"[ynm]\"\n" . $herecurr);
> > + }
> > +
> > +# discourage default \!?EXPERT
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*default \!?EXPERT/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "Avoid default turn on kernel configs by default !?EXPERT\n" . $herecurr);
> > + }
> > +
>
> I'd prefer to create a block for all the Kconfig file tests and
> avoid multiply determining if the filename includes Kconfig so
> the script runs a bit faster.
>
Thanks for your comments.
yes, the script runs faster this way.
> Also some trivial changes to the added tests with added --fix
> capability. Something like:
Thanks for posting the patch, I'll verify it and post as patch v3.
> ---
> scripts/checkpatch.pl | 139 ++++++++++++++++++++++++++++++--------------------
> 1 file changed, 85 insertions(+), 54 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 6cb99ec62000..4780149a8d30 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2934,60 +2934,98 @@ sub process {
> "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
> -# Only applies when adding the entry originally, after that we do not have
> -# sufficient context to determine whether it is indeed long enough.
> - if ($realfile =~ /Kconfig/ &&
> - # 'choice' is usually the last thing on the line (though
> - # Kconfig supports named choices), so use a word boundary
> - # (\b) rather than a whitespace character (\s)
> - $line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
> - my $length = 0;
> - my $cnt = $realcnt;
> - my $ln = $linenr + 1;
> - my $f;
> - my $is_start = 0;
> - my $is_end = 0;
> - for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
> - $f = $lines[$ln - 1];
> - $cnt-- if ($lines[$ln - 1] !~ /^-/);
> - $is_end = $lines[$ln - 1] =~ /^\+/;
> -
> - next if ($f =~ /^-/);
> - last if (!$file && $f =~ /^\@\@/);
> -
> - if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
> - $is_start = 1;
> - } elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
> - if ($lines[$ln - 1] =~ "---help---") {
> - WARN("CONFIG_DESCRIPTION",
> - "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
> +# Kconfig tests
> + if ($realfile =~ /Kconfig/) {
> + # check for Kconfig help text having a real description
> + # Only applies when adding the entry originally, after
> + # that we do not have sufficient context to determine
> + # whether it is indeed long enough.
> + # 'choice' is usually the last thing on the line (though
> + # Kconfig supports named choices), so use a word
> + # boundary (\b) rather than a whitespace character (\s)
> + if ($line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
> + my $length = 0;
> + my $cnt = $realcnt;
> + my $ln = $linenr + 1;
> + my $f;
> + my $is_start = 0;
> + my $is_end = 0;
> + for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
> + $f = $lines[$ln - 1];
> + $cnt-- if ($lines[$ln - 1] !~ /^-/);
> + $is_end = $lines[$ln - 1] =~ /^\+/;
> +
> + next if ($f =~ /^-/);
> + last if (!$file && $f =~ /^\@\@/);
> +
> + if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
> + $is_start = 1;
> + } elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
> + if ($lines[$ln - 1] =~ "---help---") {
> + WARN("CONFIG_DESCRIPTION",
> + "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
> + }
> + $length = -1;
> + }
> +
> + $f =~ s/^.//;
> + $f =~ s/#.*//;
> + $f =~ s/^\s+//;
> + next if ($f =~ /^$/);
> +
> + # This only checks context lines in the patch
> + # and so hopefully shouldn't trigger false
> + # positives, even though some of these are
> + # common words in help texts
> + if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
> + if|endif|menu|endmenu|source)\b/x) {
> + $is_end = 1;
> + last;
> }
> - $length = -1;
> + $length++;
> + }
> + if ($is_start && $is_end && $length < $min_conf_desc_length) {
> + WARN("CONFIG_DESCRIPTION",
> + "please write a paragraph that describes the config symbol fully\n" . $herecurr);
> }
> + #print "is_start<$is_start> is_end<$is_end> length<$length>\n";
> + }
>
> - $f =~ s/^.//;
> - $f =~ s/#.*//;
> - $f =~ s/^\s+//;
> - next if ($f =~ /^$/);
> -
> - # This only checks context lines in the patch
> - # and so hopefully shouldn't trigger false
> - # positives, even though some of these are
> - # common words in help texts
> - if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
> - if|endif|menu|endmenu|source)\b/x) {
> - $is_end = 1;
> - last;
> +# discourage the use of boolean for type definition attributes
> + if ($line =~ /^\+\s*\bboolean\b/) {
> + if (WARN("CONFIG_TYPE_BOOLEAN",
> + "Use of boolean is deprecated, please use bool instead\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/\bboolean\b/bool/;
> + }
> + }
> +
> +# Kconfig: discourage redundant 'default n'
> + if ($line =~ /^\+\s*default\s+n$/) {
> + if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "'default n' is the default value, no need to write it explicitly\n" . $herecurr) &&
> + $fix) {
> + fix_delete_line($fixlinenr, $rawline);
> }
> - $length++;
> }
> - if ($is_start && $is_end && $length < $min_conf_desc_length) {
> - WARN("CONFIG_DESCRIPTION",
> - "please write a paragraph that describes the config symbol fully\n" . $herecurr);
> +
> +# Kconfig: discourage quoted defaults: use default [ynm], not default "[ynm]"
> + if ($rawline =~ /^\+\s*default\s+"([ynm])"/) {
> + if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "Use 'default $1' not 'default \"$1\"'\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/\b(default\s+)"(.)"/$1$2/;
> + }
> + }
> +
> +# Kconfig: discourage using default EXPERT or !EXPERT
> + if ($line =~ /^\+\s*default\s+\!?\s*EXPERT\b/) {
> + WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "Avoid using default EXPERT\n" . $herecurr);
> }
> - #print "is_start<$is_start> is_end<$is_end> length<$length>\n";
> }
> +# End of Kconfig tests
> +
>
> # check for MAINTAINERS entries that don't have the right form
> if ($realfile =~ /^MAINTAINERS$/ &&
> @@ -3000,13 +3038,6 @@ sub process {
> }
> }
>
> -# discourage the use of boolean for type definition attributes of Kconfig options
> - if ($realfile =~ /Kconfig/ &&
> - $line =~ /^\+\s*\bboolean\b/) {
> - WARN("CONFIG_TYPE_BOOLEAN",
> - "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> - }
> -
> if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
> ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
> my $flag = $1;
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] checkpatch: add several Kconfig default value tests
@ 2019-07-05 2:06 ` Miles Chen
0 siblings, 0 replies; 5+ messages in thread
From: Miles Chen @ 2019-07-05 2:06 UTC (permalink / raw)
To: Joe Perches
Cc: Andy Whitcroft, linux-kernel, linux-mediatek, wsd_upstream, Yingjoe Chen
On Thu, 2019-07-04 at 11:49 -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 17:40 +0800, Miles Chen wrote:
> > This change adds 3 Kconfig default value tests:
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3005,6 +3005,27 @@ sub process {
> > "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> > }
> >
> > +# discourage redundant 'default n'
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*default n$/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "'default n' is the default value, no need to write it explicitly.\n" . $herecurr);
> > + }
> > +
> > +# discourage quote: use default [ynm], not default "[ynm]"
> > + if ($realfile =~ /Kconfig/ &&
> > + $rawline =~ /^\+\s*default\s*"[ynm]"/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "Use default [ynm] instead of default \"[ynm]\"\n" . $herecurr);
> > + }
> > +
> > +# discourage default \!?EXPERT
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*default \!?EXPERT/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "Avoid default turn on kernel configs by default !?EXPERT\n" . $herecurr);
> > + }
> > +
>
> I'd prefer to create a block for all the Kconfig file tests and
> avoid multiply determining if the filename includes Kconfig so
> the script runs a bit faster.
>
Thanks for your comments.
yes, the script runs faster this way.
> Also some trivial changes to the added tests with added --fix
> capability. Something like:
Thanks for posting the patch, I'll verify it and post as patch v3.
> ---
> scripts/checkpatch.pl | 139 ++++++++++++++++++++++++++++++--------------------
> 1 file changed, 85 insertions(+), 54 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 6cb99ec62000..4780149a8d30 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2934,60 +2934,98 @@ sub process {
> "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
> -# Only applies when adding the entry originally, after that we do not have
> -# sufficient context to determine whether it is indeed long enough.
> - if ($realfile =~ /Kconfig/ &&
> - # 'choice' is usually the last thing on the line (though
> - # Kconfig supports named choices), so use a word boundary
> - # (\b) rather than a whitespace character (\s)
> - $line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
> - my $length = 0;
> - my $cnt = $realcnt;
> - my $ln = $linenr + 1;
> - my $f;
> - my $is_start = 0;
> - my $is_end = 0;
> - for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
> - $f = $lines[$ln - 1];
> - $cnt-- if ($lines[$ln - 1] !~ /^-/);
> - $is_end = $lines[$ln - 1] =~ /^\+/;
> -
> - next if ($f =~ /^-/);
> - last if (!$file && $f =~ /^\@\@/);
> -
> - if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
> - $is_start = 1;
> - } elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
> - if ($lines[$ln - 1] =~ "---help---") {
> - WARN("CONFIG_DESCRIPTION",
> - "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
> +# Kconfig tests
> + if ($realfile =~ /Kconfig/) {
> + # check for Kconfig help text having a real description
> + # Only applies when adding the entry originally, after
> + # that we do not have sufficient context to determine
> + # whether it is indeed long enough.
> + # 'choice' is usually the last thing on the line (though
> + # Kconfig supports named choices), so use a word
> + # boundary (\b) rather than a whitespace character (\s)
> + if ($line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
> + my $length = 0;
> + my $cnt = $realcnt;
> + my $ln = $linenr + 1;
> + my $f;
> + my $is_start = 0;
> + my $is_end = 0;
> + for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
> + $f = $lines[$ln - 1];
> + $cnt-- if ($lines[$ln - 1] !~ /^-/);
> + $is_end = $lines[$ln - 1] =~ /^\+/;
> +
> + next if ($f =~ /^-/);
> + last if (!$file && $f =~ /^\@\@/);
> +
> + if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
> + $is_start = 1;
> + } elsif ($lines[$ln - 1] =~ /^\+\s*(?:help|---help---)\s*$/) {
> + if ($lines[$ln - 1] =~ "---help---") {
> + WARN("CONFIG_DESCRIPTION",
> + "prefer 'help' over '---help---' for new help texts\n" . $herecurr);
> + }
> + $length = -1;
> + }
> +
> + $f =~ s/^.//;
> + $f =~ s/#.*//;
> + $f =~ s/^\s+//;
> + next if ($f =~ /^$/);
> +
> + # This only checks context lines in the patch
> + # and so hopefully shouldn't trigger false
> + # positives, even though some of these are
> + # common words in help texts
> + if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
> + if|endif|menu|endmenu|source)\b/x) {
> + $is_end = 1;
> + last;
> }
> - $length = -1;
> + $length++;
> + }
> + if ($is_start && $is_end && $length < $min_conf_desc_length) {
> + WARN("CONFIG_DESCRIPTION",
> + "please write a paragraph that describes the config symbol fully\n" . $herecurr);
> }
> + #print "is_start<$is_start> is_end<$is_end> length<$length>\n";
> + }
>
> - $f =~ s/^.//;
> - $f =~ s/#.*//;
> - $f =~ s/^\s+//;
> - next if ($f =~ /^$/);
> -
> - # This only checks context lines in the patch
> - # and so hopefully shouldn't trigger false
> - # positives, even though some of these are
> - # common words in help texts
> - if ($f =~ /^\s*(?:config|menuconfig|choice|endchoice|
> - if|endif|menu|endmenu|source)\b/x) {
> - $is_end = 1;
> - last;
> +# discourage the use of boolean for type definition attributes
> + if ($line =~ /^\+\s*\bboolean\b/) {
> + if (WARN("CONFIG_TYPE_BOOLEAN",
> + "Use of boolean is deprecated, please use bool instead\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/\bboolean\b/bool/;
> + }
> + }
> +
> +# Kconfig: discourage redundant 'default n'
> + if ($line =~ /^\+\s*default\s+n$/) {
> + if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "'default n' is the default value, no need to write it explicitly\n" . $herecurr) &&
> + $fix) {
> + fix_delete_line($fixlinenr, $rawline);
> }
> - $length++;
> }
> - if ($is_start && $is_end && $length < $min_conf_desc_length) {
> - WARN("CONFIG_DESCRIPTION",
> - "please write a paragraph that describes the config symbol fully\n" . $herecurr);
> +
> +# Kconfig: discourage quoted defaults: use default [ynm], not default "[ynm]"
> + if ($rawline =~ /^\+\s*default\s+"([ynm])"/) {
> + if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "Use 'default $1' not 'default \"$1\"'\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~ s/\b(default\s+)"(.)"/$1$2/;
> + }
> + }
> +
> +# Kconfig: discourage using default EXPERT or !EXPERT
> + if ($line =~ /^\+\s*default\s+\!?\s*EXPERT\b/) {
> + WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "Avoid using default EXPERT\n" . $herecurr);
> }
> - #print "is_start<$is_start> is_end<$is_end> length<$length>\n";
> }
> +# End of Kconfig tests
> +
>
> # check for MAINTAINERS entries that don't have the right form
> if ($realfile =~ /^MAINTAINERS$/ &&
> @@ -3000,13 +3038,6 @@ sub process {
> }
> }
>
> -# discourage the use of boolean for type definition attributes of Kconfig options
> - if ($realfile =~ /Kconfig/ &&
> - $line =~ /^\+\s*\bboolean\b/) {
> - WARN("CONFIG_TYPE_BOOLEAN",
> - "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> - }
> -
> if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
> ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
> my $flag = $1;
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-07-05 2:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-04 9:40 [PATCH v2] checkpatch: add several Kconfig default value tests Miles Chen
2019-07-04 9:40 ` Miles Chen
2019-07-04 18:49 ` Joe Perches
2019-07-05 2:06 ` Miles Chen
2019-07-05 2:06 ` Miles Chen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.