On Thu, 2019-07-04 at 11:49 -0700, Joe Perches wrote:
> On Thu, 2019-07-04 at 17:40 +0800, Miles Chen wrote:
> > This change adds 3 Kconfig default value tests:
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -3005,6 +3005,27 @@ sub process {
> > "Use of boolean is deprecated, please use bool
> > instead.\n" . $herecurr);
> > }
> >
> > +# discourage redundant 'default n'
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*default n$/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "'default n' is the default value, no need to
> > write it explicitly.\n" . $herecurr);
> > + }
> > +
> > +# discourage quote: use default [ynm], not default "[ynm]"
> > + if ($realfile =~ /Kconfig/ &&
> > + $rawline =~ /^\+\s*default\s*"[ynm]"/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "Use default [ynm] instead of default \"[ynm]\"\n"
> > . $herecurr);
> > + }
> > +
> > +# discourage default \!?EXPERT
> > + if ($realfile =~ /Kconfig/ &&
> > + $line =~ /^\+\s*default \!?EXPERT/) {
> > + WARN("DEFAULT_VALUE_STYLE",
> > + "Avoid default turn on kernel configs by default
> > !?EXPERT\n" . $herecurr);
> > + }
> > +
>
> I'd prefer to create a block for all the Kconfig file tests and
> avoid multiply determining if the filename includes Kconfig so
> the script runs a bit faster.
>
Thanks for your comments.
yes, the script runs faster this way.
> Also some trivial changes to the added tests with added --fix
> capability. Something like:
Thanks for posting the patch, I'll verify it and post as patch v3.
> ---
> scripts/checkpatch.pl | 139
> ++++++++++++++++++++++++++++++--------------------
> 1 file changed, 85 insertions(+), 54 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 6cb99ec62000..4780149a8d30 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -2934,60 +2934,98 @@ sub process {
> "Do not include the paragraph about
> writing to the Free Software Foundation's mailing address from the sample GPL
> notice. The FSF has changed addresses in the past, and may do so again. Linux
> already includes a copy of the GPL.\n" . $herevet)
> }
>
> -# check for Kconfig help text having a real description
> -# 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/ &&
> - # 'choice' is usually the last thing on the line (though
> - # Kconfig supports named choices), so use a word boundary
> - # (\b) rather than a whitespace character (\s)
> - $line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
> - my $length = 0;
> - my $cnt = $realcnt;
> - my $ln = $linenr + 1;
> - my $f;
> - my $is_start = 0;
> - my $is_end = 0;
> - for (; $cnt > 0 && defined $lines[$ln - 1]; $ln++) {
> - $f = $lines[$ln - 1];
> - $cnt-- if ($lines[$ln - 1] !~ /^-/);
> - $is_end = $lines[$ln - 1] =~ /^\+/;
> -
> - next if ($f =~ /^-/);
> - last if (!$file && $f =~ /^\@\@/);
> -
> - if ($lines[$ln - 1] =~
> /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
> - $is_start = 1;
> - } elsif ($lines[$ln - 1] =~
> /^\+\s*(?:help|---help---)\s*$/) {
> - if ($lines[$ln - 1] =~ "---help---") {
> - WARN("CONFIG_DESCRIPTION",
> - "prefer 'help' over
> '---help---' for new help texts\n" . $herecurr);
> +# Kconfig tests
> + if ($realfile =~ /Kconfig/) {
> + # check for Kconfig help text having a real description
> + # Only applies when adding the entry originally, after
> + # that we do not have sufficient context to determine
> + # whether it is indeed long enough.
> + # 'choice' is usually the last thing on the line (though
> + # Kconfig supports named choices), so use a word
> + # boundary (\b) rather than a whitespace character (\s)
> + if ($line =~ /^\+\s*(?:config|menuconfig|choice)\b/) {
> + my $length = 0;
> + my $cnt = $realcnt;
> + my $ln = $linenr + 1;
> + my $f;
> + my $is_start = 0;
> + my $is_end = 0;
> + for (; $cnt > 0 && defined $lines[$ln - 1];
> $ln++) {
> + $f = $lines[$ln - 1];
> + $cnt-- if ($lines[$ln - 1] !~ /^-/);
> + $is_end = $lines[$ln - 1] =~ /^\+/;
> +
> + next if ($f =~ /^-/);
> + last if (!$file && $f =~ /^\@\@/);
> +
> + if ($lines[$ln - 1] =~
> /^\+\s*(?:bool|tristate|prompt)\s*["']/) {
> + $is_start = 1;
> + } elsif ($lines[$ln - 1] =~
> /^\+\s*(?:help|---help---)\s*$/) {
> + if ($lines[$ln - 1] =~
> "---help---") {
> +
> WARN("CONFIG_DESCRIPTION",
> + "prefer 'help'
> over '---help---' for new help texts\n" . $herecurr);
> + }
> + $length = -1;
> + }
> +
> + $f =~ s/^.//;
> + $f =~ s/#.*//;
> + $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;
> }
> - $length = -1;
> + $length++;
> + }
> + if ($is_start && $is_end && $length <
> $min_conf_desc_length) {
> + WARN("CONFIG_DESCRIPTION",
> + "please write a paragraph that
> describes the config symbol fully\n" . $herecurr);
> }
> + #print "is_start<$is_start> is_end<$is_end>
> length<$length>\n";
> + }
>
> - $f =~ s/^.//;
> - $f =~ s/#.*//;
> - $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;
> +# discourage the use of boolean for type definition attributes
> + if ($line =~ /^\+\s*\bboolean\b/) {
> + if (WARN("CONFIG_TYPE_BOOLEAN",
> + "Use of boolean is deprecated, please
> use bool instead\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~
> s/\bboolean\b/bool/;
> + }
> + }
> +
> +# Kconfig: discourage redundant 'default n'
> + if ($line =~ /^\+\s*default\s+n$/) {
> + if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "'default n' is the default value, no
> need to write it explicitly\n" . $herecurr) &&
> + $fix) {
> + fix_delete_line($fixlinenr, $rawline);
> }
> - $length++;
> }
> - if ($is_start && $is_end && $length <
> $min_conf_desc_length) {
> - WARN("CONFIG_DESCRIPTION",
> - "please write a paragraph that describes
> the config symbol fully\n" . $herecurr);
> +
> +# Kconfig: discourage quoted defaults: use default [ynm], not default "[ynm]"
> + if ($rawline =~ /^\+\s*default\s+"([ynm])"/) {
> + if (WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "Use 'default $1' not 'default
> \"$1\"'\n" . $herecurr) &&
> + $fix) {
> + $fixed[$fixlinenr] =~
> s/\b(default\s+)"(.)"/$1$2/;
> + }
> + }
> +
> +# Kconfig: discourage using default EXPERT or !EXPERT
> + if ($line =~ /^\+\s*default\s+\!?\s*EXPERT\b/) {
> + WARN("CONFIG_DEFAULT_VALUE_STYLE",
> + "Avoid using default EXPERT\n" .
> $herecurr);
> }
> - #print "is_start<$is_start> is_end<$is_end>
> length<$length>\n";
> }
> +# End of Kconfig tests
> +
>
> # check for MAINTAINERS entries that don't have the right form
> if ($realfile =~ /^MAINTAINERS$/ &&
> @@ -3000,13 +3038,6 @@ sub process {
> }
> }
>
> -# discourage the use of boolean for type definition attributes of Kconfig
> options
> - if ($realfile =~ /Kconfig/ &&
> - $line =~ /^\+\s*\bboolean\b/) {
> - WARN("CONFIG_TYPE_BOOLEAN",
> - "Use of boolean is deprecated, please use bool
> instead.\n" . $herecurr);
> - }
> -
> if (($realfile =~ /Makefile.*/ || $realfile =~ /Kbuild.*/) &&
> ($line =~ /\+(EXTRA_[A-Z]+FLAGS).*/)) {
> my $flag = $1;
>
>
