Improve and extend checkpatch.pl Kconfig help text checks
Subsystems (2)
cover Improve and extend checkpatch.pl Kconfig help text checks
Hello,
This patchset contains some improvements for the Kconfig help text check in scripts/checkconfig.pl:
- Commits 1 and 2 make the check more robust by checking menuconfig symbols and choices as well as symbols defined with 'config', and by making the detection of definition boundaries more reliable.
- Commit 3 adds a check for new '---help---'s being introduced. IMO, 'help' should be encouraged in new code.
All three commits can be applied independently.
The existing code is a bit weird in that it doesn't require symbols with "long" definitions (e.g., many selects) to have help texts. Fixing that is outside the scope of this patchset. I couldn't tell if it was deliberate.
I'm a Perl noob, so check for bad practices. :)
The changes were tested by running 'checkpatch.pl -f' on some large existing Kconfig files in the kernel and looking for false positives (e.g. arch/{x86,arm}/Kconfig).
This test file was also used, which contains some cases that confused the old code:
config BAD_1 bool "bad 1" config BAD_2 bool 'bad 2' config BAD_3 bool "bad 3" help 1 2 3 menuconfig BAD_4 bool "bad 4" help 1 2 3 config BAD_5 bool prompt "bad 5" help 1 2 3 config BAD_6 bool "bad 6" help 1 2 3 if FOO config BAD_7 bool "bad 7" help 1 2 3 endif config BAD_8 bool "bad 8" help 1 2 3 source "foo" config BAD_9 bool "bad 9" ---help--- 1 2 3 4 choice bool "bad choice" help 1 2 3 endchoice config OK_1 bool config OK_2 bool "ok 2" help 1 2 3 4 config OK_3 tristate "ok 3" help 1 2 3 4 config OK_4 tristate prompt "ok 4" help 1 2 3 4 choice bool "ok choice" help 1 2 3 4 endchoice
This now produces the following warnings:
WARNING: please write a paragraph that describes the config symbol fully #9: FILE: Kconfig.test_help_check:9:
+config BAD_1
WARNING: please write a paragraph that describes the config symbol fully #12: FILE: Kconfig.test_help_check:12:
+config BAD_2
WARNING: please write a paragraph that describes the config symbol fully #15: FILE: Kconfig.test_help_check:15:
+config BAD_3
WARNING: please write a paragraph that describes the config symbol fully #22: FILE: Kconfig.test_help_check:22:
+menuconfig BAD_4
WARNING: please write a paragraph that describes the config symbol fully #29: FILE: Kconfig.test_help_check:29:
+config BAD_5
WARNING: please write a paragraph that describes the config symbol fully #37: FILE: Kconfig.test_help_check:37:
+config BAD_6
WARNING: please write a paragraph that describes the config symbol fully #46: FILE: Kconfig.test_help_check:46:
+config BAD_7
WARNING: please write a paragraph that describes the config symbol fully #55: FILE: Kconfig.test_help_check:55:
+config BAD_8
WARNING: prefer 'help' over '---help---' for new help texts #64: FILE: Kconfig.test_help_check:64:
+config BAD_9
WARNING: please write a paragraph that describes the config symbol fully #72: FILE: Kconfig.test_help_check:72:
+choice
total: 0 errors, 10 warnings, 117 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Kconfig.test_help_check has style problems, please review.
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
Cheers,
Ulf
Ulf Magnusson (3):
checkpatch: kconfig: recognize more prompts when checking help texts
checkpatch: kconfig: check help texts for menuconfig and choice
checkpatch: kconfig: prefer 'help' over '---help---'
scripts/checkpatch.pl | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
Signature · 1 line
2.14.1
PATCH 1/3 checkpatch: kconfig: recognize more prompts when checking help texts
The check for a missing or short help text only considers symbols with a prompt, but doesn't recognize any of the following as a prompt:
bool 'foo'
tristate 'foo'
prompt "foo"
prompt 'foo'
Make the check recognize those too.
--- scripts/checkpatch.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
scripts/checkpatch.pl +1 -1
@@ -2812,7 +2812,7 @@ sub process { next if ($f =~ /^-/); last if (!$file && $f =~ /^\@\@/); - if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate)\s*\"/) { + if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { $is_start = 1; } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) { $length = -1;
Signature · 1 line
2.14.1
PATCH 2/3 checkpatch: kconfig: check help texts for menuconfig and choice
In reply to Ulf MagnussonCurrently, only Kconfig symbols are checked for a missing or short help text, and are only checked if they are defined with the 'config' keyword.
To make the check more general, extend it to also check help texts for choices and for symbols defined with the 'menuconfig' keyword.
This increases the accuracy of the check for symbols that would already have been checked as well, since e.g. a 'menuconfig' symbol after a help text will be recognized as ending the preceding symbol/choice definition.
To increase the accuracy of the check further, also recognize 'if', 'endif', 'menu', 'endmenu', 'endchoice', and 'source' as ending a symbol/choice definition.
--- scripts/checkpatch.pl | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +11 -2
@@ -2797,7 +2797,10 @@ sub process { # 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/ && - $line =~ /^\+\s*config\s+/) { + # '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;
@@ -2822,7 +2825,13 @@ sub process { $f =~ s/#.*//; $f =~ s/^\s+//; next if ($f =~ /^$/); - if ($f =~ /^\s*config\s/) { + + # 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; }
Signature · 1 line
2.14.1
Re: [PATCH 2/3] checkpatch: kconfig: check help texts for menuconfig and choice
2018-02-17 5:22 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 7 lines
Currently, only
Kconfigsymbols are checked for a missing or short help text, and are only checked if they are defined with the 'config' keyword.To make the check more general, extend it to also check help texts for choices and for symbols defined with the 'menuconfig' keyword.
Quoted 8 lines
This increases the accuracy of the check for symbols that would already have been checked as well, since e.g. a 'menuconfig' symbol after a help text will be recognized as ending the preceding symbol/choice definition.
To increase the accuracy of the check further, also recognize 'if', 'endif', 'menu', 'endmenu', 'endchoice', and 'source' as ending a symbol/choice definition.
Currently, this is not checked for the last symbol in a file. Perhaps, EOF could be also an ending of symbol definition.
This patch is a good improvement enough,
so I queued it up.
If you have an idea for further improvement,
v2 is welcome, of course.
Signature · 2 lines
Best Regards Masahiro Yamada
PATCH 3/3 checkpatch: kconfig: prefer 'help' over '---help---'
In reply to Ulf MagnussonIMO, we should discourage '---help---' for new help texts, even in cases where it would be consistent with other help texts in the file. This will help if we ever want to get rid of '---help---' in the future.
Also simplify the code to only check for exactly '---help---'. Since commit c2264564df3d ("kconfig: warn of unhandled characters in Kconfig commands"), '---help---' is a proper keyword and can only appear in that form. Prior to that commit, '---help---' working was more of a syntactic quirk.
--- scripts/checkpatch.pl | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
scripts/checkpatch.pl +5 -1
@@ -2817,7 +2817,11 @@ sub process { if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { $is_start = 1; - } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) { + } 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; }
Signature · 1 line
2.14.1
Re: [PATCH 3/3] checkpatch: kconfig: prefer 'help' over '---help---'
2018-02-17 5:22 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 22 lines
IMO, we should discourage '---help---' for new help texts, even in cases where it would be consistent with other help texts in the file. This will help if we ever want to get rid of '---help---' in the future.
Also simplify the code to only check for exactly '---help---'. Since commit
c2264564df3d("kconfig: warn of unhandled characters inKconfigcommands"), '---help---' is a proper keyword and can only appear in that form. Prior to that commit, '---help---' working was more of a syntactic quirk.Signed-off-by Ulf Magnusson <[email protected]>--- scripts/checkpatch.pl | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)scripts/checkpatch.pl +0
-0@@ -2817,7 +2817,11 @@ sub process { if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
scripts/checkpatch.pl sub process { · line 2827
Quoted 10 lines
scripts/checkpatch.pl +5
-1$is_start = 1; - } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) { + } 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; }
I applied this, but perhaps the line number for the offending part could be improved.
The warning looks like this.
WARNING: prefer 'help' over '---help---' for new help texts #1109: FILE: init/Kconfig:1109:
+config SGETMASK_SYSCALL
The #1109 points to the line of the symbol start. IMHO, it is even better to point to the '---help---' line.
Signature · 2 lines
Best Regards Masahiro Yamada
Joe Perches
maintainerQuoted 4 lines · Ulf Magnusson
Hello,
This patchset contains some improvements for the
Kconfighelp text check inscripts/checkconfig.pl:
Seems sensible enough to me.
Quoted 7 lines
- Commits 1 and 2 make the check more robust by checking menuconfig symbols and choices as well as symbols defined with 'config', and by making the detection of definition boundaries more reliable.
- Commit 3 adds a check for new '---help---'s being introduced. IMO, 'help' should be encouraged in new code.
Quoted 7 lines
All three commits can be applied independently.
The existing code is a bit weird in that it doesn't require symbols with "long" definitions (e.g., many selects) to have help texts. Fixing that is outside the scope of this patchset. I couldn't tell if it was deliberate.
I'm a Perl noob, so check for bad practices. :)
Everyone is. Seems fine.
Quoted 182 lines
The changes were tested by running 'checkpatch.pl -f' on some large existing
Kconfigfiles in the kernel and looking for false positives (e.g.arch/{x86,arm}/Kconfig).This test file was also used, which contains some cases that confused the old code:
config BAD_1 bool "bad 1" config BAD_2 bool 'bad 2' config BAD_3 bool "bad 3" help 1 2 3 menuconfig BAD_4 bool "bad 4" help 1 2 3 config BAD_5 bool prompt "bad 5" help 1 2 3 config BAD_6 bool "bad 6" help 1 2 3 if FOO config BAD_7 bool "bad 7" help 1 2 3 endif config BAD_8 bool "bad 8" help 1 2 3 source "foo" config BAD_9 bool "bad 9" ---help--- 1 2 3 4 choice bool "bad choice" help 1 2 3 endchoice config OK_1 bool config OK_2 bool "ok 2" help 1 2 3 4 config OK_3 tristate "ok 3" help 1 2 3 4 config OK_4 tristate prompt "ok 4" help 1 2 3 4 choice bool "ok choice" help 1 2 3 4 endchoiceThis now produces the following warnings:
WARNING: please write a paragraph that describes the config symbol fully #9: FILE:
Kconfig.test_help_check:9:
+configBAD_1WARNING: please write a paragraph that describes the config symbol fully #12: FILE:
Kconfig.test_help_check:12:
+configBAD_2WARNING: please write a paragraph that describes the config symbol fully #15: FILE:
Kconfig.test_help_check:15:
+configBAD_3WARNING: please write a paragraph that describes the config symbol fully #22: FILE:
Kconfig.test_help_check:22:
+menuconfigBAD_4WARNING: please write a paragraph that describes the config symbol fully #29: FILE:
Kconfig.test_help_check:29:
+configBAD_5WARNING: please write a paragraph that describes the config symbol fully #37: FILE:
Kconfig.test_help_check:37:
+configBAD_6WARNING: please write a paragraph that describes the config symbol fully #46: FILE:
Kconfig.test_help_check:46:
+configBAD_7WARNING: please write a paragraph that describes the config symbol fully #55: FILE:
Kconfig.test_help_check:55:
+configBAD_8WARNING: prefer 'help' over '---help---' for new help texts #64: FILE:
Kconfig.test_help_check:64:
+configBAD_9WARNING: please write a paragraph that describes the config symbol fully #72: FILE:
Kconfig.test_help_check:72:
+choicetotal: 0 errors, 10 warnings, 117 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Kconfig.test_help_checkhas style problems, please review.NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH inMAINTAINERS.Cheers,
UlfUlf Magnusson (3):
checkpatch: kconfig: recognize more prompts when checking help texts
checkpatch: kconfig: check help texts for menuconfig and choice
checkpatch: kconfig: prefer 'help' over '---help---'scripts/checkpatch.pl | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)
Quoted 8 lines · Joe Perches
Quoted 4 lines · Ulf Magnusson
Hello,
This patchset contains some improvements for the
Kconfighelp text check inscripts/checkconfig.pl:Seems sensible enough to me.
Signed-off-by Joe Perches <[email protected]>
Will you be taking this in yourself?
(Adding Masahiro on CC -- forgot when I sent the patchset.)
Cheers,
Ulf
2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines
Hello,
This patchset contains some improvements for the
Kconfighelp text check inscripts/checkconfig.pl:Seems sensible enough to me.
Signed-off-by Joe Perches <[email protected]>Will you be taking this in yourself?
Quoted 8 lines
(Adding Masahiro on CC -- forgot when I sent the patchset.)
Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
I am not a perl expert, but I have no objection for this series.
Thanks!
Signature · 2 lines
Best Regards Masahiro Yamada
Quoted 26 lines · Masahiro Yamada
2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines
Hello,
This patchset contains some improvements for the
Kconfighelp text check inscripts/checkconfig.pl:Seems sensible enough to me.
Signed-off-by Joe Perches <[email protected]>Will you be taking this in yourself?
Quoted 8 lines
(Adding Masahiro on CC -- forgot when I sent the patchset.)
Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlI am not a perl expert, but I have no objection for this series.
Quoted 8 lines
Thanks!
--
Best Regards
Masahiro Yamada
*Bump*
(+To: Andrew)
2018-03-06 13:52 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 33 lines
Quoted 31 lines · Masahiro Yamada
2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines
Hello,
This patchset contains some improvements for the
Kconfighelp text check inscripts/checkconfig.pl:Seems sensible enough to me.
Signed-off-by Joe Perches <[email protected]>Will you be taking this in yourself?
Quoted 8 lines
(Adding Masahiro on CC -- forgot when I sent the patchset.)
Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlI am not a perl expert, but I have no objection for this series.
Thanks!
Quoted 5 lines
Quoted 3 lines
--
Best Regards
Masahiro Yamada*Bump*
Who is addressed by "*Bump*" ?
I think patches for checkpatch.pl
are supposed to be taken care of by Andrew.
He forwards patches to Linus.
$ git log --no-merges --pretty=fuller scripts/checkpatch.pl | grep
'Commit:' | sort | uniq -c | sort -nr
555 Commit: Linus Torvalds <[email protected]>
16 Commit: Linus Torvalds <[email protected]>
4 Commit: Paul E. McKenney <[email protected]>
4 Commit: Michael S. Tsirkin <[email protected]>
2 Commit: Thomas Gleixner <[email protected]>
2 Commit: Ingo Molnar <[email protected]>
2 Commit: Greg Kroah-Hartman <[email protected]>
1 Commit: Tobin C. Harding <[email protected]>
1 Commit: Rob Herring <[email protected]>
1 Commit: Petr Mladek <[email protected]>
1 Commit: Michal Marek <[email protected]>
1 Commit: Mauro Carvalho Chehab <[email protected]>
1 Commit: Masahiro Yamada <[email protected]>
1 Commit: Lucas De Marchi <[email protected]>
1 Commit: Jiri Kosina <[email protected]>
1 Commit: Dan Williams <[email protected]>
1 Commit: Bjorn Helgaas <[email protected]>Signature · 2 lines
Best Regards Masahiro Yamada
2018-03-06 14:13 GMT+09:00 Masahiro Yamada <[email protected]>:
Quoted 76 lines
(+To: Andrew)
2018-03-06 13:52 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 33 lines
Quoted 31 lines · Masahiro Yamada
2018-02-23 10:30 GMT+09:00 Ulf Magnusson <[email protected]>:
Quoted 12 lines · Joe Perches
Quoted 8 lines · Ulf Magnusson
Quoted 4 lines
Hello,
This patchset contains some improvements for the
Kconfighelp text check inscripts/checkconfig.pl:Seems sensible enough to me.
Signed-off-by Joe Perches <[email protected]>Will you be taking this in yourself?
Quoted 8 lines
(Adding Masahiro on CC -- forgot when I sent the patchset.)
Cheers,
Ulf
--
To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.htmlI am not a perl expert, but I have no objection for this series.
Thanks!
Quoted 5 lines
Quoted 3 lines
--
Best Regards
Masahiro Yamada*Bump*
Who is addressed by "*Bump*" ?
I think patches for checkpatch.pl
are supposed to be taken care of by Andrew.He forwards patches to Linus.
$ git log --no-merges --pretty=fuller scripts/checkpatch.pl | grep 'Commit:' | sort | uniq -c | sort -nr 555 Commit: Linus Torvalds <[email protected]> 16 Commit: Linus Torvalds <[email protected]> 4 Commit: Paul E. McKenney <[email protected]> 4 Commit: Michael S. Tsirkin <[email protected]> 2 Commit: Thomas Gleixner <[email protected]> 2 Commit: Ingo Molnar <[email protected]> 2 Commit: Greg Kroah-Hartman <[email protected]> 1 Commit: Tobin C. Harding <[email protected]> 1 Commit: Rob Herring <[email protected]> 1 Commit: Petr Mladek <[email protected]> 1 Commit: Michal Marek <[email protected]> 1 Commit: Mauro Carvalho Chehab <[email protected]> 1 Commit: Masahiro Yamada <[email protected]> 1 Commit: Lucas De Marchi <[email protected]> 1 Commit: Jiri Kosina <[email protected]> 1 Commit: Dan Williams <[email protected]> 1 Commit: Bjorn Helgaas <[email protected]>
Quoted 3 lines
--
Best Regards
Masahiro Yamada
I thought Andrew would pick it up, but
nobody is taking care of this after all.
I queued it up now.
Signature · 2 lines
Best Regards Masahiro Yamada