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 <masahi...@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 <ulfali...@gmail.com>
Signed-off-by: Masahiro Yamada <masahi...@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.
