* [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test
@ 2016-06-04 5:10 Yingjoe Chen
2016-06-04 5:10 ` [PATCH v3 2/2] checkpatch: testing more config for Kconfig help text Yingjoe Chen
2016-06-06 16:43 ` [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test Joe Perches
0 siblings, 2 replies; 6+ messages in thread
From: Yingjoe Chen @ 2016-06-04 5:10 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches
Cc: linux-kernel, srv_heupstream, Andi Kleen, Paul Bolle, Yingjoe Chen
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>
---
For this series, rebase to v4.7-rc1 and dropped 'relax Kconfig help text
line number threshold' patch.
Let me know what you think. Thanks.
Change in v3:
- Rebase to v4.7-rc1
Change in v2:
- Change according to Joe Perches' suggesti
scripts/checkpatch.pl | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6750595..f5ce804 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2683,6 +2683,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*default\s*n\s*(#.*|$)/i) {
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 2/2] checkpatch: testing more config for Kconfig help text
2016-06-04 5:10 [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test Yingjoe Chen
@ 2016-06-04 5:10 ` Yingjoe Chen
2016-06-06 16:43 ` [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test Joe Perches
1 sibling, 0 replies; 6+ messages in thread
From: Yingjoe Chen @ 2016-06-04 5:10 UTC (permalink / raw)
To: Andy Whitcroft, Joe Perches
Cc: linux-kernel, srv_heupstream, Andi Kleen, Paul Bolle, Yingjoe Chen
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 | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index f5ce804..8e17593 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2646,6 +2646,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(?:---)?$/) {
@@ -2656,7 +2662,7 @@ sub process {
$f =~ s/#.*//;
$f =~ s/^\s+//;
next if ($f =~ /^$/);
- if ($f =~ /^\s*config\s/) {
+ if ($f =~ /^(?:config\s|menuconfig\s|choice\s|endchoice\s*$|comment\s|menu\s|endmenu\s*$|if\s|endif\s*$|source\s)/) {
$is_end = 1;
last;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test
2016-06-04 5:10 [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test Yingjoe Chen
2016-06-04 5:10 ` [PATCH v3 2/2] checkpatch: testing more config for Kconfig help text Yingjoe Chen
@ 2016-06-06 16:43 ` Joe Perches
2016-06-06 19:10 ` Andy Whitcroft
1 sibling, 1 reply; 6+ messages in thread
From: Joe Perches @ 2016-06-06 16:43 UTC (permalink / raw)
To: Yingjoe Chen, Andy Whitcroft, Andrew Morton
Cc: linux-kernel, srv_heupstream, Andi Kleen, Paul Bolle
On Sat, 2016-06-04 at 13:10 +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.
Is it obvious that a Kconfig has "default n" ?
This seems to work, but is this useful?
> Signed-off-by: Yingjoe Chen <yingjoe.chen@mediatek.com>
> ---
> For this series, rebase to v4.7-rc1 and dropped 'relax Kconfig help text
> line number threshold' patch.
>
> Let me know what you think. Thanks.
>
>
> Change in v3:
> - Rebase to v4.7-rc1
>
> Change in v2:
> - Change according to Joe Perches' suggesti
>
> scripts/checkpatch.pl | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 6750595..f5ce804 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2683,6 +2683,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*default\s*n\s*(#.*|$)/i) {
> + 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;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test
2016-06-06 16:43 ` [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test Joe Perches
@ 2016-06-06 19:10 ` Andy Whitcroft
2016-06-07 13:16 ` Yingjoe Chen
0 siblings, 1 reply; 6+ messages in thread
From: Andy Whitcroft @ 2016-06-06 19:10 UTC (permalink / raw)
To: Joe Perches
Cc: Yingjoe Chen, Andrew Morton, linux-kernel, srv_heupstream,
Andi Kleen, Paul Bolle
On Mon, Jun 06, 2016 at 09:43:15AM -0700, Joe Perches wrote:
> On Sat, 2016-06-04 at 13:10 +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.
>
> Is it obvious that a Kconfig has "default n" ?
> This seems to work, but is this useful?
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
I wonder particually when the submitter has supplied a comment, presumably
to tell us why it defaults to 'n'. I feel more accepting of rejecting
uncommented ones than those with.
> > + 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;
-apw
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test
2016-06-06 19:10 ` Andy Whitcroft
@ 2016-06-07 13:16 ` Yingjoe Chen
2016-06-08 19:42 ` Paul Bolle
0 siblings, 1 reply; 6+ messages in thread
From: Yingjoe Chen @ 2016-06-07 13:16 UTC (permalink / raw)
To: Andy Whitcroft
Cc: Joe Perches, Andrew Morton, linux-kernel, srv_heupstream,
Andi Kleen, Paul Bolle
Hi,
Thanks for the review.
On Mon, 2016-06-06 at 20:10 +0100, Andy Whitcroft wrote:
> On Mon, Jun 06, 2016 at 09:43:15AM -0700, Joe Perches wrote:
> > On Sat, 2016-06-04 at 13:10 +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.
> >
> > Is it obvious that a Kconfig has "default n" ?
> > This seems to work, but is this useful?
While sending patch for upstream, I saw maintainers request it to be
removed. So I think it might worth adding check to it.
Some examples from google:
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/120733.html
https://lkml.org/lkml/2013/3/16/153
https://lkml.org/lkml/2016/5/23/657
>
> > > + if ($realfile =~ /Kconfig/ &&
> > > + $line =~ /^\+\s*default\s*n\s*(#.*|$)/i) {
>
> I wonder particually when the submitter has supplied a comment, presumably
> to tell us why it defaults to 'n'. I feel more accepting of rejecting
> uncommented ones than those with.
How about change this to /^\+\s*default\s*n$/i ?
Joe.C
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test
2016-06-07 13:16 ` Yingjoe Chen
@ 2016-06-08 19:42 ` Paul Bolle
0 siblings, 0 replies; 6+ messages in thread
From: Paul Bolle @ 2016-06-08 19:42 UTC (permalink / raw)
To: Yingjoe Chen, Andy Whitcroft
Cc: Joe Perches, Andrew Morton, linux-kernel, srv_heupstream, Andi Kleen
On di, 2016-06-07 at 21:16 +0800, Yingjoe Chen wrote:
> On Mon, 2016-06-06 at 20:10 +0100, Andy Whitcroft wrote:
> > > Is it obvious that a Kconfig has "default n" ?
> > > This seems to work, but is this useful?
>
> While sending patch for upstream, I saw maintainers request it to be
> removed. So I think it might worth adding check to it.
> Some examples from google:
>
> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-September/1
> 20733.html
> https://lkml.org/lkml/2013/3/16/153
> https://lkml.org/lkml/2016/5/23/657
There's one rather subtle case where setting "default n" is, sort of,
useful. See lkml.kernel.org/r/<178407860.0zoJnDfCo1@tacticalops> .
(I seem to remember disagreeing here. Ie, in my view setting defaults
for a specific Kconfig symbol at two different places is confusing at
best. People probably weren't convinced by my objections. I also
remember diving into this by looking at the various places where a
Kconfig symbol was being set twice. I must have ended that endeavor when
it became clear to me I was't making any progress.)
Even though there's a corner case where "default n" is useful, it could
still be worth to add a checkpatch check that warns about it. But I
can't say I feel strongly about this either way.
Paul Bolle
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-08 19:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-04 5:10 [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test Yingjoe Chen
2016-06-04 5:10 ` [PATCH v3 2/2] checkpatch: testing more config for Kconfig help text Yingjoe Chen
2016-06-06 16:43 ` [PATCH v3 1/2] checkpatch: add Kconfig 'default n' test Joe Perches
2016-06-06 19:10 ` Andy Whitcroft
2016-06-07 13:16 ` Yingjoe Chen
2016-06-08 19:42 ` Paul Bolle
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).