From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29F69C2D0E4 for ; Tue, 8 Dec 2020 20:20:50 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id ED04623A53 for ; Tue, 8 Dec 2020 20:20:49 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726473AbgLHURn (ORCPT ); Tue, 8 Dec 2020 15:17:43 -0500 Received: from smtprelay0089.hostedemail.com ([216.40.44.89]:55884 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725910AbgLHUQm (ORCPT ); Tue, 8 Dec 2020 15:16:42 -0500 Received: from smtprelay.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by smtpgrave08.hostedemail.com (Postfix) with ESMTP id 83FDF18015308 for ; Tue, 8 Dec 2020 18:58:55 +0000 (UTC) Received: from filter.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay07.hostedemail.com (Postfix) with ESMTP id 2F26F181D3030; Tue, 8 Dec 2020 18:58:13 +0000 (UTC) X-Session-Marker: 6A6F6540706572636865732E636F6D X-HE-Tag: pull34_06175f1273e9 X-Filterd-Recvd-Size: 4582 Received: from XPS-9350.home (unknown [47.151.137.21]) (Authenticated sender: joe@perches.com) by omf13.hostedemail.com (Postfix) with ESMTPA; Tue, 8 Dec 2020 18:58:11 +0000 (UTC) Message-ID: <776550b7fac3870e65e903f0d8cb917dec58f7c2.camel@perches.com> Subject: Re: [RFC PATCH v2] checkpatch: rewrite Kconfig parsing From: Joe Perches To: Nicolai Fischer , linux-kernel@vger.kernel.org Cc: apw@canonical.com, johannes.czekay@fau.de, linux-kernel@i4.cs.fau.de Date: Tue, 08 Dec 2020 10:58:10 -0800 In-Reply-To: <97fb4ba0-68b3-8faf-1d98-e4990b2e63e3@fau.de> References: <97fb4ba0-68b3-8faf-1d98-e4990b2e63e3@fau.de> Content-Type: text/plain; charset="ISO-8859-1" User-Agent: Evolution 3.38.1-1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2020-12-08 at 18:18 +0100, Nicolai Fischer wrote: > Checkpatch currently only warns if the help text is too short. > To determine this the diff gets parsed for keywords starting > a new entry, but several kinds of false positives can occur with > the current implementation, especially when the config > is not well formatted. > > This patch makes the parsing more robust and includes > new warnings if: > 1) the help attribute is not specified last > 2) there is no blank line or endif before the next keyword > 3) the help text is not indented 2 spaces more than >    the attribute itself. > > Signed-off-by: Nicolai Fischer > Co-developed-by: Johannes Czekay > Signed-off-by: Johannes Czekay > --- > > This patch rewrites most of the Kconfig parsing to address > the issues mentioned in the first RFC: > > 1) search for 'help' instead of '---help---' > > I believe all the '---help---' lines have been converted to just 'help' > > so the '(?:---)?' bits here could be removed > > 2) create new warnings: > > 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. > > 3) fix handling of blank lines and rely on keywords for end of help text > > 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. > > > It has occurred to us, that kconfig-language.rst does not explicitly > specify that the help text should be the last attribute, although > this is the defacto convention. Now that checkpatch actively checks > for this, we should probably update the documentation accordingly. Generally process is either to update documentation along with with a checkpatch change or to update documentation first. Also checkpatch isn't necessarily the best tool for this. > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl [] > -# check for Kconfig help text having a real description > +# Check if Kconfig is well formatted. Warn if help text: > +# 1) is shorter than $min_conf_desc_length lines > +# 2) is not specified last > +# 3) and next keyword are not separated by a blank line or endif > +# 4) is not indented correctly >  # 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/ && [] > + my $l = $line; > + $l =~ s/^$help_indent//; > + if ($l =~ /^(?:bool|tristate|string|hex|int|prompt|default| > + depends\ on|select|imply|visible\ if|range|option)\b/x) { I think this is overly fragile. These keywords are not required to be at the same indent as help. Also as specified in scripts/kconfig/lexer.h, the kconfig specification has more keywords than the list above. In general, checkpatch does not have to be the tool of choice for verifying everything. For instance, checkpatch has a trivial check for MAINTAINERS entry ordering, but there is a complete tool called parse-maintainers.pl that verifies alphabetic section ordering. I think most of what you seem to be attempting should be in a new tool that completely understands Kconfig parsing. I suggest instead of updating checkpatch, the scripts/kconfig/ content be updated to do these things.