Messages in this thread Patch in this message | | | Subject | Re: [RFC patch] checkpatch: Add a test for long function definitions (>200 lines) | From | Joe Perches <> | Date | Sun, 17 Dec 2017 14:22:24 -0800 |
| |
On Sun, 2017-12-17 at 13:46 -0800, Linus Torvalds wrote: > On Sat, Dec 16, 2017 at 5:26 PM, Joe Perches <joe@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;
| |