linux-kernel - Re: [PATCH] checkpatch: make the line length warnings match the coding style document

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <93a470c7631d2607e7b2a12e9cc5d8e930911989.camel@perches.com>
Date:   Mon, 21 Dec 2020 20:08:20 -0800
From:   Joe Perches <joe@...ches.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     Christoph Hellwig <hch@....de>, apw@...onical.com,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kernel@...r.kernel.org,
        linux-doc <linux-doc@...r.kernel.org>,
        Miguel Ojeda <miguel.ojeda.sandonis@...il.com>
Subject: Re: [PATCH] checkpatch: make the line length warnings match the
 coding style document

On Thu, 2020-12-10 at 13:27 -0800, Joe Perches wrote:
> On Thu, 2020-12-10 at 20:09 +0000, Matthew Wilcox wrote:
> > On Thu, Dec 10, 2020 at 12:05:04PM -0800, Joe Perches wrote:
> > > Also, given the ever increasing average identifier length, strict
> > > adherence to 80 columns is sometimes just not possible without silly
> > > visual gymnastics.  The kernel now has quite a lot of 30+ character
> > > length function names, constants, and structs.
> > 
> > maybe checkpatch should warn for identifiers that are 30+ characters
> > long?  address the problem at its source ..
> 
> Hard to know when to warn as patches could just add uses of already
> existing names and emitting warnings for those would just be annoying.
> 
> Maybe something that tests long identifier additions of
> defines/functions/macros/structs but not their uses and maybe only
> then in patches and not files.
> 
> Perhaps:

Anyone care that this should be added or not added to checkpatch?

> ---
>  scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 7b086d1cd6c2..8579be987fc0 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -54,6 +54,7 @@ my @ignore = ();
>  my $help = 0;
>  my $configuration_file = ".checkpatch.conf";
>  my $max_line_length = 100;
> +my $max_identifier_length = 30;
>  my $ignore_perl_version = 0;
>  my $minimum_perl_version = 5.10.0;
>  my $min_conf_desc_length = 4;
> @@ -103,6 +104,8 @@ Options:
>    --max-line-length=n        set the maximum line length, (default $max_line_length)
>                               if exceeded, warn on patches
>                               requires --strict for use with --file
> +  --max-identifier-length=n  set the maximum identifier length, (default $max_identifier_length)
> +                             only used with patches, not output with --file
>    --min-conf-desc-length=n   set the min description length, if shorter, warn
>    --tab-size=n               set the number of spaces for tab (default $tabsize)
>    --root=PATH                PATH to the kernel tree root
> @@ -223,6 +226,7 @@ GetOptions(
>  	'show-types!'	=> \$show_types,
>  	'list-types!'	=> \$list_types,
>  	'max-line-length=i' => \$max_line_length,
> +	'max-identifier-length=i' => \$max_identifier_length,
>  	'min-conf-desc-length=i' => \$min_conf_desc_length,
>  	'tab-size=i'	=> \$tabsize,
>  	'root=s'	=> \$root,
> @@ -2489,6 +2493,7 @@ sub process {
>  	my $suppress_statement = 0;
>  
> 
>  	my %signatures = ();
> +	my %long_identifiers = ();
>  
> 
>  	# Pre-scan the patch sanitizing the lines.
>  	# Pre-scan the patch looking for any __setup documentation.
> @@ -3840,6 +3845,33 @@ sub process {
>  # check we are in a valid C source file if not then ignore this hunk
>  		next if ($realfile !~ /\.(h|c)$/);
>  
> 
> +# check for long identifiers in defines/macros/functions/structs/types/labels
> +		if (!$file) {
> +			while ($sline =~ /^\+.*\b(\w{$max_identifier_length,})\b/g) {
> +				my $id = $1;
> +				next if (exists($long_identifiers{$id}));
> +				my $use = "";
> +				if ($sline =~ /^\+\s*\#\s*define\s+$id(?!\()/) {
> +					$use = "define";
> +				} elsif ($sline =~ /^\+\s*\#\s*define\s+$id\(/) {
> +					$use = "function-like macro";
> +				} elsif ($sline =~ /^\+\s*(?!define)$Declare?$id\s*\(/) {
> +					$use = "function";
> +				} elsif ($sline =~ /^\+\s*(struct|union|enum)\s+$id\b/) {
> +					$use = "$1";
> +				} elsif ($sline =~ /^\+\s*$Declare$id\b/) {
> +					$use = "declaration";
> +				} elsif ($sline =~ /^\+\s*$id\s*:\s*$/) {
> +					$use = "label";
> +				}
> +				if ($use ne "") {
> +					$long_identifiers{$id} = $id;
> +					WARN("LONG_IDENTIFIER",
> +					     "$use '$id' is " . length($id) . " characters - avoid using identifiers with $max_identifier_length+ characters\n" . $herecurr);
> +				}
> +			}
> +		}
> +
>  # check for unusual line ending [ or (
>  		if ($line =~ /^\+.*([\[\(])\s*$/) {
>  			CHK("OPEN_ENDED_LINE",
>