* Re: [PATCH] checkpatch: make the line length warnings match the coding style document [not found] <20201210082251.2717564-1-hch@lst.de> @ 2020-12-10 20:05 ` Joe Perches 2020-12-10 20:09 ` Matthew Wilcox 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2020-12-10 20:05 UTC (permalink / raw) To: Christoph Hellwig, apw, Linus Torvalds, Andrew Morton Cc: linux-kernel, linux-doc, Miguel Ojeda 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@lst.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"; > } ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: make the line length warnings match the coding style document 2020-12-10 20:05 ` [PATCH] checkpatch: make the line length warnings match the coding style document Joe Perches @ 2020-12-10 20:09 ` Matthew Wilcox 2020-12-10 21:27 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Matthew Wilcox @ 2020-12-10 20:09 UTC (permalink / raw) To: Joe Perches Cc: Christoph Hellwig, apw, Linus Torvalds, Andrew Morton, linux-kernel, linux-doc, Miguel Ojeda 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 .. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: make the line length warnings match the coding style document 2020-12-10 20:09 ` Matthew Wilcox @ 2020-12-10 21:27 ` Joe Perches 2020-12-22 4:08 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2020-12-10 21:27 UTC (permalink / raw) To: Matthew Wilcox Cc: Christoph Hellwig, apw, Linus Torvalds, Andrew Morton, linux-kernel, linux-doc, Miguel Ojeda 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: --- 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", ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: make the line length warnings match the coding style document 2020-12-10 21:27 ` Joe Perches @ 2020-12-22 4:08 ` Joe Perches 2020-12-22 13:12 ` Christoph Hellwig 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2020-12-22 4:08 UTC (permalink / raw) To: Matthew Wilcox Cc: Christoph Hellwig, apw, Linus Torvalds, Andrew Morton, linux-kernel, linux-doc, Miguel Ojeda 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", > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: make the line length warnings match the coding style document 2020-12-22 4:08 ` Joe Perches @ 2020-12-22 13:12 ` Christoph Hellwig 2020-12-22 16:22 ` Joe Perches 0 siblings, 1 reply; 7+ messages in thread From: Christoph Hellwig @ 2020-12-22 13:12 UTC (permalink / raw) To: Joe Perches Cc: Matthew Wilcox, Christoph Hellwig, apw, Linus Torvalds, Andrew Morton, linux-kernel, linux-doc, Miguel Ojeda On Mon, Dec 21, 2020 at 08:08:20PM -0800, Joe Perches wrote: > 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? It is pretty useless. What we need is a patch that doesn't make people uselessly add overly long lines against the intent of the coding style document. I have submitted a pretty reasonable one, and I'm open to alternatives, but we need to to stop people submitting code that does not fit the coding style all the time because checkpatch doesn't complain. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: make the line length warnings match the coding style document 2020-12-22 13:12 ` Christoph Hellwig @ 2020-12-22 16:22 ` Joe Perches 2020-12-23 8:29 ` Christoph Hellwig 0 siblings, 1 reply; 7+ messages in thread From: Joe Perches @ 2020-12-22 16:22 UTC (permalink / raw) To: Christoph Hellwig Cc: Matthew Wilcox, apw, Linus Torvalds, Andrew Morton, linux-kernel, linux-doc, Miguel Ojeda On Tue, 2020-12-22 at 14:12 +0100, Christoph Hellwig wrote: > On Mon, Dec 21, 2020 at 08:08:20PM -0800, Joe Perches wrote: > > 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? > > It is pretty useless. Maybe so, if only because I chose a high value for the max id length to avoid controversy. I would prefer something like 20. > What we need is a patch that doesn't make people > uselessly add overly long lines against the intent of the coding style > document. I have submitted a pretty reasonable one, and I'm open to > alternatives, but we need to to stop people submitting code that does > not fit the coding style all the time because checkpatch doesn't > complain. Having checkpatch complain about > 80 column lines didn't stop patches before, likely it wouldn't stop patches now. Emitting yet more messages for trivial lines > 80 columns is also against the intent of the commit that changed the line length maximum. commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144 checkpatch/coding-style: deprecate 80-column warning The effect of your patch might as well revert the checkpatch portion of that commit. I think that's not a great idea for the reason in the commit message. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] checkpatch: make the line length warnings match the coding style document 2020-12-22 16:22 ` Joe Perches @ 2020-12-23 8:29 ` Christoph Hellwig 0 siblings, 0 replies; 7+ messages in thread From: Christoph Hellwig @ 2020-12-23 8:29 UTC (permalink / raw) To: Joe Perches Cc: Christoph Hellwig, Matthew Wilcox, apw, Linus Torvalds, Andrew Morton, linux-kernel, linux-doc, Miguel Ojeda On Tue, Dec 22, 2020 at 08:22:06AM -0800, Joe Perches wrote: > Having checkpatch complain about > 80 column lines didn't stop > patches before, likely it wouldn't stop patches now. > > Emitting yet more messages for trivial lines > 80 columns is also > against the intent of the commit that changed the line length maximum. It certainly helped. Since that checkpatch change I waste a lot more of my time on finding all this crap, and people are confused because they only rely on checkpatch. Other maintainers are similarly annoyed or just silently fix things up. Right now this is making things much worse. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-23 8:30 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20201210082251.2717564-1-hch@lst.de>
2020-12-10 20:05 ` [PATCH] checkpatch: make the line length warnings match the coding style document Joe Perches
2020-12-10 20:09 ` Matthew Wilcox
2020-12-10 21:27 ` Joe Perches
2020-12-22 4:08 ` Joe Perches
2020-12-22 13:12 ` Christoph Hellwig
2020-12-22 16:22 ` Joe Perches
2020-12-23 8:29 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).