Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1384220 > unrolled thread
| Started by | Yingjoe Chen <yingjoe.chen@mediatek.com> |
|---|---|
| First post | 2016-04-21 15:30 +0200 |
| Last post | 2016-04-22 16:20 +0200 |
| Articles | 7 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH 1/3] checkpatch: add Kconfig 'default n' test Yingjoe Chen <yingjoe.chen@mediatek.com> - 2016-04-21 15:30 +0200
[PATCH 2/3] checkpatch: testing more config for Kconfig help text Yingjoe Chen <yingjoe.chen@mediatek.com> - 2016-04-21 15:30 +0200
Re: [PATCH 2/3] checkpatch: testing more config for Kconfig help text Joe Perches <joe@perches.com> - 2016-04-21 19:10 +0200
Re: [PATCH 2/3] checkpatch: testing more config for Kconfig help text Yingjoe Chen <yingjoe.chen@mediatek.com> - 2016-04-22 15:40 +0200
Re: [PATCH 1/3] checkpatch: add Kconfig 'default n' test Joe Perches <joe@perches.com> - 2016-04-21 19:20 +0200
Re: [PATCH 1/3] checkpatch: add Kconfig 'default n' test Paul Bolle <pebolle@tiscali.nl> - 2016-04-21 20:30 +0200
Re: [PATCH 1/3] checkpatch: add Kconfig 'default n' test Yingjoe Chen <yingjoe.chen@mediatek.com> - 2016-04-22 16:20 +0200
| From | Yingjoe Chen <yingjoe.chen@mediatek.com> |
|---|---|
| Date | 2016-04-21 15:30 +0200 |
| Subject | [PATCH 1/3] checkpatch: add Kconfig 'default n' test |
| Message-ID | <rqmQb-1lx-33@gated-at.bofh.it> |
If a Kconfig config option doesn't specify 'default', the default
will be n. Adding 'default n' is unnecessary.
Add a test to warn about this.
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
scripts/checkpatch.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d574d13..1c43dc1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2600,6 +2600,13 @@ sub process {
"Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
}
+# discourage the use of default n
+ if ($realfile =~ /Kconfig/ &&
+ $line =~ /^\+\s*\bdefault\b\s*n\s*$/) {
+ WARN("CONFIG_DEFAULT_N",
+ "Use of default n is unnecessary, default is n when omitted.\n" . $herecurr);
+ }
+
if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
my $flag = $1;
--
1.9.1
[toc] | [next] | [standalone]
| From | Yingjoe Chen <yingjoe.chen@mediatek.com> |
|---|---|
| Date | 2016-04-21 15:30 +0200 |
| Subject | [PATCH 2/3] checkpatch: testing more config for Kconfig help text |
| Message-ID | <rqmQc-1lx-61@gated-at.bofh.it> |
| In reply to | #1384220 |
Current help text check only check a config option if it is followed
by another config.
Adding check for help text if the next entry is menuconfig, choice/
endchoice, comment, menu/endmenu, if/endif, source or end of file.
Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
---
scripts/checkpatch.pl | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 1c43dc1..2bf4499 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2563,6 +2563,12 @@ sub process {
next if ($f =~ /^-/);
last if (!$file && $f =~ /^\@\@/);
+ if ($f !~ /^[+\- ]/) {
+ # End of file
+ $is_end = 1;
+ last;
+ }
+
if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) {
$is_start = 1;
} elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
@@ -2573,7 +2579,10 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
- if ($f =~ /^\s*config\s/) {
+ if ($f =~ /^\s*config\s/ || $f =~ /^\s*menuconfig\s/ || $f =~ /^\s*choice\s/ ||
+ $f =~ /^\s*endchoice$/ || $f =~ /^\s*comment\s/ || $f =~ /^\s*menu\s/ ||
+ $f =~ /^\s*endmenu$/ || $f =~ /^\s*if\s/ || $f =~ /^\s*endif$/ ||
+ $f =~ /^\s*source\s/) {
$is_end = 1;
last;
}
--
1.9.1
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-04-21 19:10 +0200 |
| Subject | Re: [PATCH 2/3] checkpatch: testing more config for Kconfig help text |
| Message-ID | <rqqh4-46m-3@gated-at.bofh.it> |
| In reply to | #1384223 |
On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> Current help text check only check a config option if it is followed
> by another config.
> Adding check for help text if the next entry is menuconfig, choice/
> endchoice, comment, menu/endmenu, if/endif, source or end of file.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2563,6 +2563,12 @@ sub process {
> next if ($f =~ /^-/);
> last if (!$file && $f =~ /^\@\@/);
>
> + if ($f !~ /^[+\- ]/) {
> + # End of file
> + $is_end = 1;
> + last;
> + }
> +
> if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) {
> $is_start = 1;
> } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
> @@ -2573,7 +2579,10 @@ sub process {
> $f =~ s/#.*//;
> $f =~ s/^\s+//;
> next if ($f =~ /^$/);
> - if ($f =~ /^\s*config\s/) {
> + if ($f =~ /^\s*config\s/ || $f =~ /^\s*menuconfig\s/ || $f =~ /^\s*choice\s/ ||
> + $f =~ /^\s*endchoice$/ || $f =~ /^\s*comment\s/ || $f =~ /^\s*menu\s/ ||
> + $f =~ /^\s*endmenu$/ || $f =~ /^\s*if\s/ || $f =~ /^\s*endif$/ ||
> + $f =~ /^\s*source\s/) {
> $is_end = 1;
> last;
> }
This seems relatively verbose.
Also, because there's a substitution above that strips leading spaces,
the "^\s*" uses are unnecessary and can be simplified to ^
Maybe:
if ($f =~ /^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu$|if\s|endif\s*$|source\s)
[toc] | [prev] | [next] | [standalone]
| From | Yingjoe Chen <yingjoe.chen@mediatek.com> |
|---|---|
| Date | 2016-04-22 15:40 +0200 |
| Subject | Re: [PATCH 2/3] checkpatch: testing more config for Kconfig help text |
| Message-ID | <rqJtp-2Ga-15@gated-at.bofh.it> |
| In reply to | #1384434 |
On Thu, 2016-04-21 at 10:06 -0700, Joe Perches wrote:
> On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> > Current help text check only check a config option if it is followed
> > by another config.
> > Adding check for help text if the next entry is menuconfig, choice/
> > endchoice, comment, menu/endmenu, if/endif, source or end of file.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2563,6 +2563,12 @@ sub process {
> > next if ($f =~ /^-/);
> > last if (!$file && $f =~ /^\@\@/);
> >
> > + if ($f !~ /^[+\- ]/) {
> > + # End of file
> > + $is_end = 1;
> > + last;
> > + }
> > +
> > if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) {
> > $is_start = 1;
> > } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
> > @@ -2573,7 +2579,10 @@ sub process {
> > $f =~ s/#.*//;
> > $f =~ s/^\s+//;
> > next if ($f =~ /^$/);
> > - if ($f =~ /^\s*config\s/) {
> > + if ($f =~ /^\s*config\s/ || $f =~ /^\s*menuconfig\s/ || $f =~ /^\s*choice\s/ ||
> > + $f =~ /^\s*endchoice$/ || $f =~ /^\s*comment\s/ || $f =~ /^\s*menu\s/ ||
> > + $f =~ /^\s*endmenu$/ || $f =~ /^\s*if\s/ || $f =~ /^\s*endif$/ ||
> > + $f =~ /^\s*source\s/) {
> > $is_end = 1;
> > last;
> > }
>
> This seems relatively verbose.
> Also, because there's a substitution above that strips leading spaces,
> the "^\s*" uses are unnecessary and can be simplified to ^
>
> Maybe:
> if ($f =~ /^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu$|if\s|endif\s*$|source\s)
>
Thanks
Strip apply to trailing spaces as well, will change this to:
if ($f =~ /^(?:config\s|menuconfig\s|choice\s|endchoice$|comment\s|menu\s|endmenu$|if\s|endif$|source\s)/)
Joe.C
[toc] | [prev] | [next] | [standalone]
| From | Joe Perches <joe@perches.com> |
|---|---|
| Date | 2016-04-21 19:20 +0200 |
| Message-ID | <rqqqK-4ao-11@gated-at.bofh.it> |
| In reply to | #1384220 |
On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> If a Kconfig config option doesn't specify 'default', the default
> will be n. Adding 'default n' is unnecessary.
>
> Add a test to warn about this.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -2600,6 +2600,13 @@ sub process {
> "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> }
>
> +# discourage the use of default n
> + if ($realfile =~ /Kconfig/ &&
> + $line =~ /^\+\s*\bdefault\b\s*n\s*$/) {
maybe line =~ /^\+\s*default\s+n/i
Also, there are some oddities like:
arch/mips/cavium-octeon/Kconfig: default "n"
and a dozen or so uses like
default n if <foo>
[toc] | [prev] | [next] | [standalone]
| From | Paul Bolle <pebolle@tiscali.nl> |
|---|---|
| Date | 2016-04-21 20:30 +0200 |
| Message-ID | <rqrwu-587-17@gated-at.bofh.it> |
| In reply to | #1384442 |
On do, 2016-04-21 at 10:16 -0700, Joe Perches wrote:
> Also, there are some oddities like:
>
> arch/mips/cavium-octeon/Kconfig: default "n"
For v4.6-rc4:
$ git grep -n -e "default\s\+\"[mny]\"" -- "*Kconfig*"
arch/mips/Kconfig:2232: default "y"
arch/mips/Kconfig:2237: default "y"
arch/mips/Kconfig:2257: default "y"
arch/mips/Kconfig:2262: default "y"
arch/mips/cavium-octeon/Kconfig:5: default "n"
arch/mips/cavium-octeon/Kconfig:30: default "n"
arch/mips/cavium-octeon/Kconfig:39: default "y"
arch/mips/cavium-octeon/Kconfig:46: default "y"
arch/mips/cavium-octeon/Kconfig:53: default "y"
arch/mips/cavium-octeon/Kconfig:60: default "y"
arch/mips/cavium-octeon/Kconfig:67: default "y"
arch/mips/cavium-octeon/Kconfig:74: default "y"
arch/powerpc/Kconfig:435: default "y" if PPC_POWERNV
arch/powerpc/Kconfig:658: default "y" if PPC_POWERNV
arch/powerpc/Kconfig:869: default "n"
drivers/crypto/Kconfig:142: default "m"
drivers/misc/Kconfig:784: default "n"
drivers/rapidio/devices/Kconfig:8: default "n"
These appear to behave as intended, but I still think the quotes should
be dropped. (A brave soul might want to dive into the kconfig internals
and see whether they can be made less liberal in their parsing of
Kconfig files in this regard. I'm not volunteering.)
Paul Bolle
[toc] | [prev] | [next] | [standalone]
| From | Yingjoe Chen <yingjoe.chen@mediatek.com> |
|---|---|
| Date | 2016-04-22 16:20 +0200 |
| Message-ID | <rqK65-3fM-11@gated-at.bofh.it> |
| In reply to | #1384442 |
On Thu, 2016-04-21 at 10:16 -0700, Joe Perches wrote:
> On Thu, 2016-04-21 at 21:28 +0800, Yingjoe Chen wrote:
> > If a Kconfig config option doesn't specify 'default', the default
> > will be n. Adding 'default n' is unnecessary.
> >
> > Add a test to warn about this.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -2600,6 +2600,13 @@ sub process {
> > "Use of boolean is deprecated, please use bool instead.\n" . $herecurr);
> > }
> >
> > +# discourage the use of default n
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*\bdefault\b\s*n\s*$/) {
>
> maybe line =~ /^\+\s*default\s+n/i
This might match
default NIOS2
also this won't match lines with comment.
Will change to $line =~ /^\+\s*default\s*n\s*(#.*|$)/i
> Also, there are some oddities like:
>
> arch/mips/cavium-octeon/Kconfig: default "n"
I think this belong to another (new) test: If an option is bool or
tristate, the default value shouldn't be " quoted.
> and a dozen or so uses like
>
> default n if <foo>
>
I'm not sure about this. With if, it did provide more information.
According to Documentation/kbuild/kconfig-language.txt:
" A config option can have any number of default values. If multiple
default values are visible, only the first defined one is active."
So the following example will be different when the first one is
removed.
default n if <foo>
default y if <bar>
of course we could do the following, but one could argue the above one
is more readable.
default y if <bar> && ! <foo>
Joe.C
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web