Messages in this thread | | | Subject | Re: [RFC PATCH] checkpatch: correctly detect lines of help text | From | Joe Perches <> | Date | Wed, 02 Dec 2020 10:54:24 -0800 |
| |
On Wed, 2020-12-02 at 19:27 +0100, Nicolai Fischer wrote: > Currently, checkpatch uses keywords to determine the end > of a Kconfig help message which leads to false positives: > > 1) if a line of the help text starts with any of the keywords, e.g. if: > > +config FOO > + help > + help text > + if condition > + previous line causes warning > + last line. > > 2) if the help attribute is not specified last, checkpatch counts > other attributes like depends on towards the line count: > > +config FOO > + help > + bool "no help message, but passes checkpatch" > + default n > + depends on SYSFS > + depends on MULTIUSER
Perhaps it'd be better to create a new warning when the help text block is not the last block of the config section. Maybe warn when a blank line or endif is not the separator to the next keyword. Maybe warn when the next line after help is not indented 2 more spaces than the help line.
> This patch fixes this behavior by using the indentation to determine > the end of the help message.
This probably won't work, see below:
> The code responsible for counting the lines of the help message > seems overly complicated and we could rewrite it entirely > in order to be more clear and compact if requested.
Yes please.
> This could potentially be addressed in the warning message, > though we are happy for any input on this. [] > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > @@ -3234,6 +3234,7 @@ sub process { > my $f; > my $is_start = 0; > my $is_end = 0; > + my $help_indent; > for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) { > $f = $lines[$ln - 1]; > $cnt-- if ($lines[$ln - 1] !~ /^-/); > @@ -3245,7 +3246,12 @@ sub process { > if ($lines[$ln - 1] =~ /^\+\s*(?:bool|tristate|prompt)\s*["']/) { > $is_start = 1; > } elsif ($lines[$ln - 1] =~ /^\+\s*(?:---)?help(?:---)?$/) {
I believe all the '---help---' lines have been converted to just 'help' so the '(?:---)?' bits here could be removed.
See:
commit 22a4ac026c15eba961883ed8466cb341e0447de1 Author: Masahiro Yamada <masahiroy@kernel.org> Date: Wed Jun 17 12:02:20 2020 +0900
Revert "checkpatch: kconfig: prefer 'help' over '---help---'"
This reverts commit 84af7a6194e493fae312a2b7fa5a3b51f76d9282.
The conversion is done.
Cc: Ulf Magnusson <ulfalizer@gmail.com> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> @@ -3253,14 +3259,13 @@ sub process { > $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; > + # Help text ends if a line has a smaller indentation > + # than the first line of the message > + if (defined $help_indent) { > + if ($lines[$ln - 1] !~ /^\+$help_indent\S+/) { > + $is_end = 1; > + last; > + }
Indentation can vary in the help blocks. For instance:
arch/Kconfig: help arch/Kconfig- Functions will have the stack-protector canary logic added in> arch/Kconfig- of the following conditions: arch/Kconfig- arch/Kconfig- - local variable's address used as part of the right hand sid> arch/Kconfig- assignment or function argument arch/Kconfig- - local variable is an array (or union containing an array), arch/Kconfig- regardless of array type or length arch/Kconfig- - uses register local variables arch/Kconfig-
This doesn't allow blank lines for multi-paragraph help text either.
I think keyword parsing is necessary and some false positives are inevitable as the parsing logic in a line-by-line analyzer will always be incomplete.
| |