Messages in this thread | | | Subject | Re: [RFC PATCH v2] checkpatch: rewrite Kconfig parsing | From | Joe Perches <> | Date | Tue, 08 Dec 2020 10:58:10 -0800 |
| |
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 <nicolai.fischer@fau.de> > Co-developed-by: Johannes Czekay <johannes.czekay@fau.de> > Signed-off-by: Johannes Czekay <johannes.czekay@fau.de> > --- > > 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.
| |