On Sun, 2017-12-17 at 13:46 -0800, Linus Torvalds wrote:
> On Sat, Dec 16, 2017 at 5:26 PM, Joe Perches <j...@perches.com> wrote:
> > >
> > > I'm not expecting you to be able to write a perl script that checks
> > > the first line, but we have way too many 200-plus line functions in
> > > the kernel. I'd like a warning on anything over 200 lines (a factor
> > > of 4 over Linus's stated goal).
> >
> > In response to Matthew's request:
> >
> > This is a possible checkpatch warning for long
> > function definitions.
>
> So I'm not sure a line count makes sense.
>
> Sometimes long functions can be sensible, if they are basically just
> one big case-statement or similar.
>
> Looking at one of your examples: futex_requeue() is indeed a long
> function, but that's mainly because it has a lot of comments about
> exactly what is going on, and while it only has one (fairly small)
> case statement, the rest of it is very similar (ie "in this case, do
> XYZ").
>
> Another case I looked at - try_to_unmap_one() - had very similar
> behavior. It's long, but it's not long for the wrong reasons.
>
> And yes, "copy_process()" is disgusting, and probably _could_ be split
> up a bit, but at the same time the bulk of the lines there really is
> just the "initialize all the parts of the "struct task_struct".
>
> And other times, I suspect even a 50-line function is way too dense,
> just because it's doing crazy things.
>
> So I have a really hard time with some arbitrary line limit. At eh
> very least, I think it should ignore comments and whitespace lines.
That part is easy enough to do. (below)
> And yes, some real "complexity analysis" might give a much more sane
> limit, but I don't even know what that would be or how it would work.
I suspect there are better tools (like gnu complexity)
for this and I'm not at all tied to this as a checkpatch
feature.
btw: futex_requeue line count is now 140 so it doesn't warn.
---
scripts/checkpatch.pl | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 168687ae24fa..99c065f90360 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -59,6 +59,7 @@ my $conststructsfile = "$D/const_structs.checkpatch";
my $typedefsfile = "";
my $color = "auto";
my $allow_c99_comments = 1;
+my $max_function_length = 200;
sub help {
my ($exitcode) = @_;
@@ -2202,6 +2203,8 @@ sub process {
my $realcnt = 0;
my $here = '';
my $context_function; #undef'd unless there's a known function
+ my $context_function_start;
+ my $context_function_lines;
my $in_comment = 0;
my $comment_edge = 0;
my $first_line = 0;
@@ -2341,6 +2344,8 @@ sub process {
} else {
undef $context_function;
}
+ undef $context_function_start;
+ $context_function_lines = 0;
next;
# track the line number as we move through the hunk, note that
@@ -3200,11 +3205,25 @@ sub process {
if ($sline =~ /^\+\{\s*$/ &&
$prevline =~
/^\+(?:(?:(?:$Storage|$Inline)\s*)*\s*$Type\s*)?($Ident)\(/) {
$context_function = $1;
+ $context_function_start = $realline;
+ $context_function_lines = 0;
+ }
+
+# if in a function, count the non-blank lines
+ if (defined ($context_function) && $sline !~ /^[ \+]\s*$/) {
+ $context_function_lines++;
}
# check if this appears to be the end of function declaration
if ($sline =~ /^\+\}\s*$/) {
+ if (defined($context_function_start) &&
+ $context_function_lines > $max_function_length) {
+ WARN("LONG_FUNCTION",
+ "'$context_function' function definition
is " . $context_function_lines . " statement lines, perhaps refactor\n" .
$herecurr);
+ }
undef $context_function;
+ undef $context_function_start;
+ $context_function_lines = 0;
}
# check indentation of any line with a bare else
@@ -5983,6 +6002,8 @@ sub process {
defined $stat &&
$stat =~
/^.\s*(?:$Storage\s+)?$Type\s*($Ident)\s*$balanced_parens\s*{/s) {
$context_function = $1;
+ $context_function_start = $realline;
+ $context_function_lines = 0;
# check for multiline function definition with misplaced open brace
my $ok = 0;
