Message-ID: <1513474017.31581.22.camel@perches.com>
Date:   Sat, 16 Dec 2017 17:26:57 -0800
From:   Joe Perches <joe@...ches.com>
To:     Andrew Morton <akpm@...ux-foundation.org>
Cc:     linux-kernel@...r.kernel.org, Dan Carpenter <error27@...il.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jonathan Corbet <corbet@....net>,
        Matthew Wilcox <willy@...radead.org>
Subject: [RFC patch] checkpatch: Add a test for long function definitions
 (>200 lines)

On Mon, 2017-12-11 at 14:43 -0800, Matthew Wilcox wrote:
>  - There's no warning for the first paragraph of section 6:
> 
> 6) Functions
> ------------
> 
> Functions should be short and sweet, and do just one thing.  They should
> fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
> as we all know), and do one thing and do that well.
> 
>    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.

Running against the last 10000 git commits, there
are no false positives though perhaps there are
some false negatives.

These are the matches in the last 10000 commits:

     1	   227	42e0442f8a237d3de9ea3f2dd2be2739e6db7fdb:1157: WARNING: 'ir_lirc_ioctl' function definition is 206 lines, perhaps refactor
     2	   552	148abd3b5b146021a637d36ac5c0ee91cd4ad520:790: WARNING: 'tda18250_set_params' function definition is 206 lines, perhaps refactor
     3	  1352	123e25c4a5658be27f08ed0fb85ade34683e5dc7:366: WARNING: 'cudbg_fill_meminfo' function definition is 252 lines, perhaps refactor
     4	  2688	62d591a8e00cc349e6a9efb87efac9548f178624:462: WARNING: 'program_watermarks' function definition is 232 lines, perhaps refactor
     5	  6171	d2ddc776a4581d900fc3bdc7803b403daae64d88:3530: WARNING: 'afs_select_fileserver' function definition is 283 lines, perhaps refactor
     6	  9135	2f4b411a3d6766e6362ffbf00e0495a2dfe92507:962: WARNING: 'i40e_parse_cls_flower' function definition is 242 lines, perhaps refactor
     7	  9450	fd708b81d972a0714b02a60eb4792fdbf15868c4:1094: WARNING: 'btrfs_ref_tree_mod' function definition is 201 lines, perhaps refactor

Running against files in mm lib and kernel, there are
52 functions that exceed 200 lines.

$ git ls-files -- mm kernel lib | \
  xargs ./scripts/checkpatch.pl -f  --types=long_function --terse --quiet --no-summary | \
  cat -n
     1	kernel/audit.c:1447: WARNING: 'audit_receive_msg' function definition is 308 lines, perhaps refactor
     2	kernel/auditsc.c:713: WARNING: 'audit_filter_rules' function definition is 273 lines, perhaps refactor
     3	kernel/bpf/core.c:1285: WARNING: '___bpf_prog_run' function definition is 508 lines, perhaps refactor
     4	kernel/bpf/verifier.c:2240: WARNING: 'adjust_scalar_min_max_vals' function definition is 218 lines, perhaps refactor
     5	kernel/bpf/verifier.c:4064: WARNING: 'do_check' function definition is 288 lines, perhaps refactor
     6	kernel/debug/debug_core.c:682: WARNING: 'kgdb_cpu_enter' function definition is 214 lines, perhaps refactor
     7	kernel/debug/kdb/kdb_io.c:422: WARNING: 'kdb_read' function definition is 217 lines, perhaps refactor
     8	kernel/debug/kdb/kdb_io.c:850: WARNING: 'vkdb_printf' function definition is 297 lines, perhaps refactor
     9	kernel/fork.c:2033: WARNING: 'copy_process' function definition is 454 lines, perhaps refactor
    10	kernel/futex.c:705: WARNING: 'get_futex_key' function definition is 204 lines, perhaps refactor
    11	kernel/futex.c:2135: WARNING: 'futex_requeue' function definition is 283 lines, perhaps refactor
    12	kernel/irq/manage.c:1488: WARNING: '__setup_irq' function definition is 354 lines, perhaps refactor
    13	kernel/locking/locktorture.c:1050: WARNING: 'lock_torture_init' function definition is 201 lines, perhaps refactor
    14	kernel/locking/qspinlock.c:505: WARNING: 'queued_spin_lock_slowpath' function definition is 210 lines, perhaps refactor
    15	kernel/locking/rtmutex.c:796: WARNING: 'rt_mutex_adjust_prio_chain' function definition is 348 lines, perhaps refactor
    16	kernel/power/swap.c:873: WARNING: 'save_image_lzo' function definition is 205 lines, perhaps refactor
    17	kernel/power/swap.c:1469: WARNING: 'load_image_lzo' function definition is 312 lines, perhaps refactor
    18	kernel/ptrace.c:1104: WARNING: 'ptrace_request' function definition is 221 lines, perhaps refactor
    19	kernel/sched/core.c:4254: WARNING: '_setscheduler' function definition is 238 lines, perhaps refactor
    20	kernel/sched/fair.c:8722: WARNING: 'load_balance' function definition is 261 lines, perhaps refactor
    21	kernel/trace/trace_kprobe.c:839: WARNING: 'create_trace_kprobe' function definition is 207 lines, perhaps refactor
    22	kernel/trace/trace_uprobe.c:564: WARNING: 'create_trace_uprobe' function definition is 202 lines, perhaps refactor
    23	lib/asn1_decoder.c:518: WARNING: 'asn1_ber_decoder' function definition is 347 lines, perhaps refactor
    24	lib/assoc_array.c:790: WARNING: 'assoc_array_insert_into_terminal_node' function definition is 312 lines, perhaps refactor
    25	lib/assoc_array.c:1721: WARNING: 'assoc_array_gc' function definition is 266 lines, perhaps refactor
    26	lib/decompress_bunzip2.c:513: WARNING: 'get_next_block' function definition is 357 lines, perhaps refactor
    27	lib/lz4/lz4_compress.c:456: WARNING: 'LZ4_compress_generic' function definition is 280 lines, perhaps refactor
    28	lib/lz4/lz4_decompress.c:335: WARNING: 'LZ4_decompress_generic' function definition is 283 lines, perhaps refactor
    29	lib/lz4/lz4hc_compress.c:579: WARNING: 'LZ4HC_compress_generic' function definition is 241 lines, perhaps refactor
    30	lib/lzo/lzo1x_decompress_safe.c:261: WARNING: 'lzo1x_decompress_safe' function definition is 223 lines, perhaps refactor
    31	lib/mpi/mpi-pow.c:329: WARNING: 'mpi_powm' function definition is 291 lines, perhaps refactor
    32	lib/raid6/recov_avx512.c:230: WARNING: 'raid6_2data_recov_avx512' function definition is 201 lines, perhaps refactor
    33	lib/vsprintf.c:3109: WARNING: 'vsscanf' function definition is 275 lines, perhaps refactor
    34	lib/zlib_inflate/inffast.c:347: WARNING: 'inflate_fast' function definition is 258 lines, perhaps refactor
    35	lib/zlib_inflate/inflate.c:740: WARNING: 'zlib_inflate' function definition is 422 lines, perhaps refactor
    36	lib/zlib_inflate/inftrees.c:315: WARNING: 'zlib_inflate_table' function definition is 292 lines, perhaps refactor
    37	lib/zstd/compress.c:830: WARNING: 'ZSTD_compressSequences_internal' function definition is 243 lines, perhaps refactor
    38	lib/zstd/zstd_opt.h:697: WARNING: 'ZSTD_compressBlock_opt_generic' function definition is 289 lines, perhaps refactor
    39	lib/zstd/zstd_opt.h:1012: WARNING: 'ZSTD_compressBlock_opt_extDict_generic' function definition is 311 lines, perhaps refactor
    40	mm/compaction.c:958: WARNING: 'isolate_migratepages_block' function definition is 268 lines, perhaps refactor
    41	mm/filemap.c:2303: WARNING: 'generic_file_buffered_read' function definition is 251 lines, perhaps refactor
    42	mm/khugepaged.c:1560: WARNING: 'collapse_shmem' function definition is 262 lines, perhaps refactor
    43	mm/ksm.c:1755: WARNING: 'stable_tree_search' function definition is 236 lines, perhaps refactor
    44	mm/memory.c:3080: WARNING: 'do_swap_page' function definition is 210 lines, perhaps refactor
    45	mm/mmap.c:968: WARNING: '__vma_adjust' function definition is 287 lines, perhaps refactor
    46	mm/nommu.c:1424: WARNING: 'do_mmap' function definition is 223 lines, perhaps refactor
    47	mm/page-writeback.c:1829: WARNING: 'balance_dirty_pages' function definition is 268 lines, perhaps refactor
    48	mm/rmap.c:1609: WARNING: 'try_to_unmap_one' function definition is 275 lines, perhaps refactor
    49	mm/shmem.c:1911: WARNING: 'shmem_getpage_gfp' function definition is 315 lines, perhaps refactor
    50	mm/swapfile.c:2253: WARNING: 'try_to_unuse' function definition is 222 lines, perhaps refactor
    51	mm/swapfile.c:3325: WARNING: 'SYSCALL_DEFINE2' function definition is 230 lines, perhaps refactor
    52	mm/vmscan.c:1297: WARNING: 'shrink_page_list' function definition is 411 lines, perhaps refactor

Anyone think this function line length test is useful?
Should it be longer? shorter? not exist at all?

---
 scripts/checkpatch.pl | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4306b7616cdd..523aead40b87 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,7 @@ sub process {
 	my $realcnt = 0;
 	my $here = '';
 	my $context_function;		#undef'd unless there's a known function
+	my $context_function_linenum;
 	my $in_comment = 0;
 	my $comment_edge = 0;
 	my $first_line = 0;
@@ -2341,6 +2343,7 @@ sub process {
 			} else {
 				undef $context_function;
 			}
+			undef $context_function_linenum;
 			next;
 
 # track the line number as we move through the hunk, note that
@@ -3200,11 +3203,18 @@ sub process {
 		if ($sline =~ /^\+\{\s*$/ &&
 		    $prevline =~ /^\+(?:(?:(?:$Storage|$Inline)\s*)*\s*$Type\s*)?($Ident)\(/) {
 			$context_function = $1;
+			$context_function_linenum = $realline;
 		}
 
 # check if this appears to be the end of function declaration
 		if ($sline =~ /^\+\}\s*$/) {
+			if (defined($context_function_linenum) &&
+			    ($realline - $context_function_linenum) > $max_function_length) {
+				WARN("LONG_FUNCTION",
+				     "'$context_function' function definition is " . ($realline - $context_function_linenum) . " lines, perhaps refactor\n" . $herecurr);
+			}
 			undef $context_function;
+			undef $context_function_linenum;
 		}
 
 # check indentation of any line with a bare else
@@ -5983,6 +5993,7 @@ sub process {
 		    defined $stat &&
 		    $stat =~ /^.\s*(?:$Storage\s+)?$Type\s*($Ident)\s*$balanced_parens\s*{/s) {
 			$context_function = $1;
+			$context_function_linenum = $realline;
 
 # check for multiline function definition with misplaced open brace
 			my $ok = 0;

Powered by blists - more mailing lists