Please fix or revert: [PATCH] checkpatch: add --strict tests for braces, comments and casts

This recent checkpatch commit, added in v3.4-rc1:

aad4f6149831 checkpatch: add --strict tests for braces, comments and casts

made the default checkpatch run complain about the following perfectly fine multi-line comment block:

(quoted diff) +9 -0

+               rdp->qlen_lazy = 0;
+               rdp->qlen = 0;
+
+               /*
+                * Adopt all callbacks.  The outgoing CPU was in no shape
+                * to advance them, so make them all go through a full
+                * grace period.
+                */
+               *receive_rdp->nxttail[RCU_NEXT_TAIL] = rdp->nxtlist;

With:

CHECK: Don't begin block comments with only a /* line, use /* comment...

#80: FILE: kernel/rcutree.c:1428:

(quoted diff) +2 -0

+
+		/*

Which is an obviously bogus warning: the comment is perfectly fine and it has the form that Documentation/CodingStyle specifically recommends:

|                 Chapter 8: Commenting
|
| [...]
|
| The preferred style for long (multi-line) comments is:
|
|        /*
|         * This is the preferred style for multi-line
|         * comments in the Linux kernel source code.
|         * Please use it consistently.
|         *
|         * Description:  A column of asterisks on the left side,
|         * with beginning and ending almost-blank lines.
|         */
|

Please turn this new warning off by default, or fix the check, or revert it. (The revert below applies cleanly on top of latest -git and solves the warning for me.)

Thanks,

Ingo

This reverts commit aad4f61498314d41d047ea2161b7bd7237b72d33.

Signed-off-by Ingo Molnar <[email protected]>

---
 scripts/checkpatch.pl |   40 ++++++++--------------------------------
 1 files changed, 8 insertions(+), 32 deletions(-)

scripts/checkpatch.pl +8 -33

index de639ee..6217093 100755
@@ -1864,17 +1864,6 @@ sub process {
 			}
 		}
 
-		if ($line =~ /^\+.*\*[ \t]*\)[ \t]+/) {
-			CHK("SPACING",
-			    "No space is necessary after a cast\n" . $hereprev);
-		}
-
-		if ($rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
-		    $prevrawline =~ /^\+[ \t]*$/) {
-			CHK("BLOCK_COMMENT_STYLE",
-			    "Don't begin block comments with only a /* line, use /* comment...\n" . $hereprev);
-		}
-
 # check for spaces at the beginning of a line.
 # Exceptions:
 #  1) within comments

@@ -2987,8 +2976,7 @@ sub process {
 			#print "chunks<$#chunks> linenr<$linenr> endln<$endln> level<$level>\n";
 			#print "APW: <<$chunks[1][0]>><<$chunks[1][1]>>\n";
 			if ($#chunks > 0 && $level == 0) {
-				my @allowed = ();
-				my $allow = 0;
+				my $allowed = 0;
 				my $seen = 0;
 				my $herectx = $here . "\n";
 				my $ln = $linenr - 1;

@@ -2999,7 +2987,6 @@ sub process {
 					my ($whitespace) = ($cond =~ /^((?:\s*\n[+-])*\s*)/s);
 					my $offset = statement_rawlines($whitespace) - 1;
 
-					$allowed[$allow] = 0;
 					#print "COND<$cond> whitespace<$whitespace> offset<$offset>\n";
 
 					# We have looked at and allowed this specific line.

@@ -3012,34 +2999,23 @@ sub process {
 
 					$seen++ if ($block =~ /^\s*{/);
 
-					#print "cond<$cond> block<$block> allowed<$allowed[$allow]>\n";
+					#print "cond<$cond> block<$block> allowed<$allowed>\n";
 					if (statement_lines($cond) > 1) {
 						#print "APW: ALLOWED: cond<$cond>\n";
-						$allowed[$allow] = 1;
+						$allowed = 1;
 					}
 					if ($block =~/\b(?:if|for|while)\b/) {
 						#print "APW: ALLOWED: block<$block>\n";
-						$allowed[$allow] = 1;
+						$allowed = 1;
 					}
 					if (statement_block_size($block) > 1) {
 						#print "APW: ALLOWED: lines block<$block>\n";
-						$allowed[$allow] = 1;
+						$allowed = 1;
 					}
-					$allow++;
 				}
-				if ($seen) {
-					my $sum_allowed = 0;
-					foreach (@allowed) {
-						$sum_allowed += $_;
-					}
-					if ($sum_allowed == 0) {
-						WARN("BRACES",
-						     "braces {} are not necessary for any arm of this statement\n" . $herectx);
-					} elsif ($sum_allowed != $allow &&
-						 $seen != $allow) {
-						CHK("BRACES",
-						    "braces {} should be used on all arms of this statement\n" . $herectx);
-					}
+				if ($seen && !$allowed) {
+					WARN("BRACES",
+					     "braces {} are not necessary for any arm of this statement\n" . $herectx);
 				}
 			}
 		}
--

Quoted 3 lines · Ingo Molnar

aad4f6149831 checkpatch: add --strict tests for braces, comments and casts
made the default checkpatch run complain about the following perfectly fine multi-line comment block:

Quoted 3 lines · Ingo Molnar

aad4f6149831 checkpatch: add --strict tests for braces, comments and casts
made the default checkpatch run complain about the following perfectly fine multi-line comment block:

Revert the --strict test for the old preferred block comment style in drivers/net and net/

Quoted 2 lines · Joe Perches

Revert the --strict test for the old preferred block comment style in drivers/net and net/

Emphatic NAK.

Quoted 7 lines · Ingo Molnar

Quoted 2 lines · Joe Perches

Revert the --strict test for the old preferred block comment style in drivers/net and net/

Emphatic NAK.

Ingo, I don't think you understood. That test reverts the thing you wanted reverted wrt the block comments.

Yeah, I was confused: I judged it by the title and description and assumed it was only doing it for drivers/net/ and net/.

This recent checkpatch commit, added in v3.4-rc1:

aad4f6149831 checkpatch: add --strict tests for braces, comments and casts

made the default checkpatch run complain about the following perfectly fine multi-line comment block:

I would like to formally propose that we change CodingStyle so that our comments are of the form:

       /* XXX
        * YYY
        */

So that we can save one line of vertical space.

It looks unbalanced. If you want to save that vertical space, just use C++ style comments instead, for chissake! Then you save *two* lines, without the unbalanced thing, and just write it as

    // XXX
    // YYY

which is much preferred.

From: Ingo Molnar <[email protected]>
Date: Mon, 16 Apr 2012 11:17:20 +0200

Quoted 9 lines

This recent checkpatch commit, added in v3.4-rc1:

aad4f6149831 checkpatch: add --strict tests for braces, comments and casts

made the default checkpatch run complain about the following perfectly fine multi-line comment block:

Review

Quoted 9 lines

(quoted diff) +9 -0

+               rdp->qlen_lazy = 0;
+               rdp->qlen = 0;
+
+               /*
+                * Adopt all callbacks.  The outgoing CPU was in no shape
+                * to advance them, so make them all go through a full
+                * grace period.
+                */
+               *receive_rdp->nxttail[RCU_NEXT_TAIL] = rdp->nxtlist;

I would like to formally propose that we change CodingStyle so that our comments are of the form:

	/* XXX
	 * YYY
	 */

So that we can save one line of vertical space.

We've adopted this all across the networking, and the code looks a lot nicer as well as allowing more actual code onto a single window of text.

If you don't "see" it as ugly it's simply because your brain is not wired up to see 3D/2D geometry as a significant source of information. A significant portion (I'd guesstimate a narrow majority) of kernel hackers *does* see 2D/3D layout details in code and finds inconsistencies in them counterproductive.

( It's roughly the same distinction that makes some people
love the typographic layout of the iPhone/iPad while others consider it unnecessary bling. )

Quoted 11 lines

If you don't "see" it as ugly it's simply because your brain is not wired up to see 3D/2D geometry as a significant source of information. A significant portion (I'd guesstimate a narrow majority) of kernel hackers *does* see 2D/3D layout details in code and finds inconsistencies in them counterproductive.

( It's roughly the same distinction that makes some people
love the typographic layout of the iPhone/iPad while others consider it unnecessary bling. )

Another example are the recent Google+ layout changes. Those who don't see 2D details found it an unnecessary waste of screen real estate.

To me and many others the new layout, while sparser, is actually noticeably less taxing to read, because information is structured in such a "geometrically obvious" way.

I guess us visual retards will have to find a new web site to use then, thanks for the lesson Ingo.

From: Ingo Molnar <[email protected]>
Date: Mon, 16 Apr 2012 22:34:51 +0200

Quoted 18 lines · Ingo Molnar

Quoted 11 lines

If you don't "see" it as ugly it's simply because your brain is not wired up to see 3D/2D geometry as a significant source of information. A significant portion (I'd guesstimate a narrow majority) of kernel hackers *does* see 2D/3D layout details in code and finds inconsistencies in them counterproductive.

( It's roughly the same distinction that makes some people
love the typographic layout of the iPhone/iPad while others consider it unnecessary bling. )

Another example are the recent Google+ layout changes. Those who don't see 2D details found it an unnecessary waste of screen real estate.

Quoted 3 lines

To me and many others the new layout, while sparser, is actually noticeably less taxing to read, because information is structured in such a "geometrically obvious" way.

I guess us visual retards will have to find a new web site to use then, thanks for the lesson Ingo.

1) It's butt ugly and it violates every basic principle of typography. Saving a line is often counter-productive, a well placed whitespace (vertical or horizontal) will often increase readability. Key is good balance.

If you don't "see" it as ugly it's simply because your brain is not wired up to see 3D/2D geometry as a significant source of information. A significant portion (I'd guesstimate a narrow majority) of kernel hackers *does* see 2D/3D layout details in code and finds inconsistencies in them counterproductive.

If you don't "see" it as ugly it's simply because your brain is not wired up to see 3D/2D geometry as a significant source of information.

From: Ingo Molnar <[email protected]>
Date: Mon, 16 Apr 2012 22:26:54 +0200

Quoted 3 lines

If you don't "see" it as ugly it's simply because your brain is not wired up to see 3D/2D geometry as a significant source of information.

I'm sorry that I'm so handicapped that I cannot match your visual capabilitites.