Re: [PATCH] checkpatch: make the line length warnings match the coding style document
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.
Re: [PATCH] checkpatch: make the line length warnings match the coding style document
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 +, 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.
Re: [PATCH] checkpatch: make the line length warnings match the coding style document
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 +, 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.
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 +, 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=nset 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=PATHPATH 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",
>
Re: [PATCH] checkpatch: make the line length warnings match the coding style document
On Thu, 2020-12-10 at 20:09 +, 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=nset 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=PATHPATH 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",
Re: [PATCH] checkpatch: make the line length warnings match the coding style document
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 ..
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
> ---
> 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,
>
