Skip to contentSkip to conversation

scripts/checkpatch.pl: Improve guidance for LONG_LINE

PATCH In review Josh Triplett 2 participants 2 messages Jun 26, 2014 Checkpatch

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 scripts/checkpatch.pl: Improve guidance for LONG_LINE

Currently, LONG_LINE just informs the user about the line length, leaving them to shorten the line. Too many users run checkpatch and blindly follow its recommendation by splitting long lines, which almost invariably results in worse code. On rare occasions, the line-width limit encourages sensible refactoring of nested code into functions, but more frequently it just results in painfully over-wrapped code.

Improve the guidance by detecting long lines that start with 4+ tabs and explicitly suggesting simplification or refactoring in that case.

This does not introduce or remove any warnings; it just changes the text of a warning that checkpatch would already emit, to emphasize a preference for simplifying or refactoring complex, deeply nested code, rather than just wrapping it but leaving it complex.

---

Of the ~267000 80+ character lines in the current kernel source, this new guidance would trigger on ~67000 of them.

Other things I tested, but did not include in this patch, to minimize controversy or subjectivity: flagging long lines with 2 or more logical operations (&& or ||) to suggest wrapping at the operators, and flagging long lines with 30+ character identifiers to suggest consideration of whether the identifier could remain self-documenting if shortened.

 scripts/checkpatch.pl | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
scripts/checkpatch.pl +10 -2
index 010b18e..6e82f19 100755
@@ -2146,8 +2146,16 @@ sub process {
 		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
 		    $length > $max_line_length)
 		{
-			WARN("LONG_LINE",
-			     "line over $max_line_length characters\n" . $herecurr);
+			if ($line =~ /^\+\t{4,}/) {
+				WARN("LONG_LINE_DEEP_NESTING",
+				     "line over $max_line_length characters with excessive nesting (4+ tabs)\n"
+				     . "Consider simplifying or refactoring to eliminate excessive nesting.\n"
+				     . $herecurr);
+			} else {
+				WARN("LONG_LINE",
+				     "line over $max_line_length characters\n"
+				     . $herecurr);
+			}
 		}
 
 # Check for user-visible strings broken across lines, which breaks the ability
Signature · 1 line
2.0.0
maintainer
#1 lore raw
Quoted 9 lines · Josh Triplett

Currently, LONG_LINE just informs the user about the line length, leaving them to shorten the line. Too many users run checkpatch and blindly follow its recommendation by splitting long lines, which almost invariably results in worse code. On rare occasions, the line-width limit encourages sensible refactoring of nested code into functions, but more frequently it just results in painfully over-wrapped code.

Improve the guidance by detecting long lines that start with 4+ tabs and explicitly suggesting simplification or refactoring in that case.

[]

scripts/checkpatch.pl

Quoted 1 line
scripts/checkpatch.pl +0 -0

[]

scripts/checkpatch.pl sub process { · line 2153

Quoted 11 lines
scripts/checkpatch.pl +5 -2
@@ -2146,8 +2146,16 @@ sub process {
 		    $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
 		    $length > $max_line_length)
 		{
-			WARN("LONG_LINE",
-			     "line over $max_line_length characters\n" . $herecurr);
+			if ($line =~ /^\+\t{4,}/) {
+				WARN("LONG_LINE_DEEP_NESTING",
+				     "line over $max_line_length characters with excessive nesting (4+ tabs)\n"
+				     . "Consider simplifying or refactoring to eliminate excessive nesting.\n"
+				     . $herecurr);

This is not the test you want.

This is also emitting on lines that are
merely continued like

			pr_warning(format,
				   arg1,
				   arg2,
				   arg3
				   ...
				   argN);

Better would be to test only for the lines that increase indents (like the DEEP_INDENTATION test)

$line =~ /^\+\t{4,}(?:if|for|while|do|switch)\b/

This is the treewide distribution I get for the lines that generally produce extra indentation:

608432 1
242180 2
71260 3
16953 4
3688 5

    758 6
    187 7
     74 8
     23 9
     17 10
      8 11
      4 12
      1 13