From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757611AbdLQWW3 (ORCPT ); Sun, 17 Dec 2017 17:22:29 -0500 Received: from smtprelay0188.hostedemail.com ([216.40.44.188]:56819 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1757348AbdLQWW2 (ORCPT ); Sun, 17 Dec 2017 17:22:28 -0500 X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 30,2,0,,d41d8cd98f00b204,joe@perches.com,:::::::::::,RULES_HIT:41:355:379:541:599:800:960:973:982:988:989:1260:1277:1311:1313:1314:1345:1359:1373:1437:1515:1516:1518:1535:1544:1593:1594:1711:1730:1747:1777:1792:2197:2199:2393:2553:2559:2562:2828:3138:3139:3140:3141:3142:3354:3622:3653:3865:3866:3867:3868:3870:3871:3872:3873:3874:4321:5007:6120:6691:7903:10004:10226:10848:11026:11232:11658:11914:12043:12291:12295:12438:12555:12663:12679:12740:12760:12895:12986:13255:13439:13618:14093:14097:14181:14659:14721:21063:21080:21221:21324:21433:21451:21505:21611:21627:30034:30054:30060:30070:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:2,LUA_SUMMARY:none X-HE-Tag: day04_325a78ba91727 X-Filterd-Recvd-Size: 5623 Message-ID: <1513549344.31581.33.camel@perches.com> Subject: Re: [RFC patch] checkpatch: Add a test for long function definitions (>200 lines) From: Joe Perches To: Linus Torvalds Cc: Andrew Morton , Linux Kernel Mailing List , Dan Carpenter , Jonathan Corbet , Matthew Wilcox Date: Sun, 17 Dec 2017 14:22:24 -0800 In-Reply-To: References: <20171208223654.GP5858@dastard> <1512838818.26342.7.camel@perches.com> <20171211214300.GT5858@dastard> <1513030348.3036.5.camel@perches.com> <20171211224301.GA3925@bombadil.infradead.org> <1513474017.31581.22.camel@perches.com> Content-Type: text/plain; charset="ISO-8859-1" X-Mailer: Evolution 3.26.1-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 2017-12-17 at 13:46 -0800, Linus Torvalds wrote: > On Sat, Dec 16, 2017 at 5:26 PM, Joe Perches 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;