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=unavailable 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 60403C4361B for ; Thu, 10 Dec 2020 20:06:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0D67723A7C for ; Thu, 10 Dec 2020 20:06:18 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404559AbgLJUGR (ORCPT ); Thu, 10 Dec 2020 15:06:17 -0500 Received: from smtprelay0158.hostedemail.com ([216.40.44.158]:60664 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S2393719AbgLJUGC (ORCPT ); Thu, 10 Dec 2020 15:06:02 -0500 Received: from filter.hostedemail.com (clb03-v110.bra.tucows.net [216.40.38.60]) by smtprelay05.hostedemail.com (Postfix) with ESMTP id D474318026130; Thu, 10 Dec 2020 20:05:06 +0000 (UTC) X-Session-Marker: 6A6F6540706572636865732E636F6D X-HE-Tag: kick42_05118bb273fb X-Filterd-Recvd-Size: 6732 Received: from XPS-9350.home (unknown [47.151.137.21]) (Authenticated sender: joe@perches.com) by omf17.hostedemail.com (Postfix) with ESMTPA; Thu, 10 Dec 2020 20:05:05 +0000 (UTC) Message-ID: Subject: Re: [PATCH] checkpatch: make the line length warnings match the coding style document From: Joe Perches To: Christoph Hellwig , apw@canonical.com, Linus Torvalds , Andrew Morton Cc: linux-kernel@vger.kernel.org, linux-doc , Miguel Ojeda Date: Thu, 10 Dec 2020 12:05:04 -0800 In-Reply-To: <20201210082251.2717564-1-hch@lst.de> References: <20201210082251.2717564-1-hch@lst.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 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 > --- >  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"; >   }