Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1518815662.13169.27.camel@perches.com>
Subject: Re: [PATCH 0/3] Improve and extend checkpatch.pl Kconfig help text
 checks
From:   Joe Perches <joe@perches.com>
To:     Ulf Magnusson <ulfalizer@gmail.com>, apw@canonical.com,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     linux-kernel@vger.kernel.org, linux-kbuild@vger.kernel.org
Date:   Fri, 16 Feb 2018 13:14:22 -0800
In-Reply-To: <20180216202255.25307-1-ulfalizer@gmail.com>
References: <20180216202255.25307-1-ulfalizer@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Fri, 2018-02-16 at 21:22 +0100, Ulf Magnusson wrote:
> Hello,
> 
> This patchset contains some improvements for the Kconfig help text check in
> scripts/checkconfig.pl:

Seems sensible enough to me.
Signed-off-by: Joe Perches <joe@perches.com>

>  - 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. :)

Everyone is.  Seems fine.

> 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(-)
>