| 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
| ||
|
Date: Thu, 10 Dec 2020 12:05:04 -0800
From: Joe Perches <joe@...ches.com>
To: Christoph Hellwig <hch@....de>, apw@...onical.com,
Linus Torvalds <torvalds@...ux-foundation.org>,
Andrew Morton <akpm@...ux-foundation.org>
Cc: 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 09:22 +0100, Christoph Hellwig wrote:
> Add a new informational message that lines <= 80 chars are still
> preffered. Without this people unfortunately auto format code way over
> 80 lines without the required benefit for readability.
In general, I agree with some of the concept, though I think 80
columns is sometimes overly restrictive.
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.
(these generic searches probably have some false positives and negatives)
# defines
$ git grep -P -oh 'define\s+\w{30,}(?!\()' -- '*.[ch]' | sort | uniq | wc -l
1009715
(A lot or even most of those are autogenerated and never used)
# function like macros
$ git grep -P -oh 'define\s+\w{30,}\(' -- '*.[ch]' | sort | uniq | wc -l
6583
# functions
$ git grep -P -oh '\b(?!define)\w+\s+\w{30,}\s*\(' -- '*.[ch]' | sort | uniq | wc -l
31091
# structs
$ git grep -P -oh 'struct\s+\w{30,}' -- '*.[ch]' | sort | uniq | wc -l
3250
Using those identifiers with 80 column restrictions is just stupid.
A logical complexity analysis, and/or something that takes those long
identifiers into account rather than a simple line length test might
be more appropriate though more difficult to create.
Perhaps this should be enabled/disabled similarly to --check where
the reporting is not done unless specifically requested via something
like --info.
And in that case, maybe it should just as well be a --strict CHK test.
> Signed-off-by: Christoph Hellwig <hch@....de>
> ---
> scripts/checkpatch.pl | 41 ++++++++++++++++++++++++++++++-----------
> 1 file changed, 30 insertions(+), 11 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index fab38b493cef79..d937889a5fe3b2 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 $preferred_line_length = 80;
> my $ignore_perl_version = 0;
> my $minimum_perl_version = 5.10.0;
> my $min_conf_desc_length = 4;
> @@ -2228,6 +2229,16 @@ sub WARN {
> }
> return 0;
> }
> +sub INFO {
> + my ($type, $msg) = @_;
> +
> + if (report("INFO", $type, $msg)) {
> + our $clean = 0;
> + our $cnt_info++;
> + return 1;
> + }
> + return 0;
> +}
> sub CHK {
> my ($type, $msg) = @_;
>
>
> @@ -2396,6 +2407,7 @@ sub process {
> our $cnt_lines = 0;
> our $cnt_error = 0;
> our $cnt_warn = 0;
> + our $cnt_info = 0;
> our $cnt_chk = 0;
>
>
> # Trace the real file/line as we go.
> @@ -3343,15 +3355,15 @@ sub process {
> # if LONG_LINE is ignored, the other 2 types are also ignored
> #
>
>
> - if ($line =~ /^\+/ && $length > $max_line_length) {
> + if ($line =~ /^\+/ && $length > $preferred_line_length) {
> my $msg_type = "LONG_LINE";
>
>
> # Check the allowed long line types first
>
>
> # logging functions that end in a string that starts
> - # before $max_line_length
> + # before $preferred_line_length
> if ($line =~ /^\+\s*$logFunctions\s*\(\s*(?:(?:KERN_\S+\s*|[^"]*))?($String\s*(?:|,|\)\s*;)\s*)$/ &&
> - length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $max_line_length) {
> + length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $preferred_line_length) {
> $msg_type = "";
>
>
> # lines with only strings (w/ possible termination)
> @@ -3371,23 +3383,30 @@ sub process {
>
>
> # Otherwise set the alternate message types
>
>
> - # a comment starts before $max_line_length
> + # a comment starts before $preferred_line_length
> } elsif ($line =~ /($;[\s$;]*)$/ &&
> - length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $max_line_length) {
> + length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $preferred_line_length) {
> $msg_type = "LONG_LINE_COMMENT"
>
>
> - # a quoted string starts before $max_line_length
> + # a quoted string starts before $preferred_line_length
> } elsif ($sline =~ /\s*($String(?:\s*(?:\\|,\s*|\)\s*;\s*))?)$/ &&
> - length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $max_line_length) {
> + length(expand_tabs(substr($line, 1, length($line) - length($1) - 1))) <= $preferred_line_length) {
> $msg_type = "LONG_LINE_STRING"
> }
>
>
> if ($msg_type ne "" &&
> (show_type("LONG_LINE") || show_type($msg_type))) {
> - my $msg_level = \&WARN;
> - $msg_level = \&CHK if ($file);
> - &{$msg_level}($msg_type,
> + my $msg_level = \&CHK;
> +
> + if ($line =~ /^\+/ && $length <= $max_line_length) {
> + $msg_level = \&INFO if (!$file);
> + &{$msg_level}($msg_type,
> + "line length of $length exceeds preferred $preferred_line_length columns\n" . $herecurr);
> + } else {
> + $msg_level = \&WARN if (!$file);
> + &{$msg_level}($msg_type,
> "line length of $length exceeds $max_line_length columns\n" . $herecurr);
> + }
> }
> }
>
>
> @@ -7015,7 +7034,7 @@ sub process {
> print report_dump();
> if ($summary && !($clean == 1 && $quiet == 1)) {
> print "$filename " if ($summary_file);
> - print "total: $cnt_error errors, $cnt_warn warnings, " .
> + print "total: $cnt_error errors, $cnt_warn warnings, $cnt_info informational, " .
> (($check)? "$cnt_chk checks, " : "") .
> "$cnt_lines lines checked\n";
> }
Powered by blists - more mailing lists