[PATCH] checkpatch: don't encourage new code to use "networking" style comments

[PATCH] checkpatch: don't encourage new code to use "networking" style comments

[Brian Norris]

Our glorious leader has made his opinion known [1]: the "networking"
comment style is not useful for new code. While the same rules as usual
still apply -- e.g., don't unnecessarily churn existing code, and follow
existing practice within files -- that doesn't mean that checkpatch
should be enforcing that for entire directories. Among other reasons,
this can cause automatic patch generators to do the exact wrong thing:
convert perfectly good existing code into the "networking style", just
because it's in a similar directory.

[1] http://lkml.iu.edu/hypermail/linux/kernel/1607.1/00627.html
    Re: [patch] crypto: sha256-mb - cleanup a || vs | typo

And funny side note from that thread:
http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1181110.html

Ingo:
"Btw., as a historic reference, there is nothing sacred about the
'networking comments coding style': I was there (way too many years ago)
when that comment style was introduced by Alan Cox's first TCP/IP code
drop, and it was little more than just a random inconsistency that
people are now treating as gospel..."

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 scripts/checkpatch.pl | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index baa3c7be04ad..fb6b6570d275 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2991,15 +2991,6 @@ sub process {
 		}
 
 # Block comment styles
-# Networking with an initial /*
-		if ($realfile =~ m@^(drivers/net/|net/)@ &&
-		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
-		    $rawline =~ /^\+[ \t]*\*/ &&
-		    $realline > 2) {
-			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
-			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
-		}
-
 # Block comments use * on subsequent lines
 		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
 		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*
-- 
2.13.0.rc0.306.g87b477812d-goog

Re: [PATCH] checkpatch: don't encourage new code to use "networking" style comments

[Joe Perches]

On Fri, 2017-04-28 at 10:55 -0700, Brian Norris wrote:
> Our glorious leader has made his opinion known [1]: the "networking"
> comment style is not useful for new code.

<shrug>  and yet nothing was done.

I think _very_ few people concern themselves one way
or another.

I believe the only person that actually cares about
the networking
comment style is David Miller.

> While the same rules as usual
> still apply -- e.g., don't unnecessarily churn existing code, and follow
> existing practice within files -- that doesn't mean that checkpatch
> should be enforcing that for entire directories. Among other reasons,
> this can cause automatic patch generators to do the exact wrong thing:
> convert perfectly good existing code into the "networking style", just
> because it's in a similar directory.

I believe the patch generator you are referring to is
checkpatch.

And checkpatch doesn't actually offer to "--fix" any
comment style.  It just bleats a message.

I don't know of another tool that proposes patches
that vary comment styles based on directory.

Re: [PATCH] checkpatch: don't encourage new code to use "networking" style comments

[Brian Norris]

On Fri, Apr 28, 2017 at 11:24:18AM -0700, Joe Perches wrote:
> On Fri, 2017-04-28 at 10:55 -0700, Brian Norris wrote:
> > Our glorious leader has made his opinion known [1]: the "networking"
> > comment style is not useful for new code.
> 
> <shrug>  and yet nothing was done.
> 
> I think _very_ few people concern themselves one way
> or another.

Right, so why should checkpatch complain? You're adding one more thing
to my mental filter whenever I run checkpatch.

> I believe the only person that actually cares about
> the networking
> comment style is David Miller.

Which is why I've CC'd him. If even *he* doesn't care about having this
warning in checkpatch, then why should anyone else?

> > While the same rules as usual
> > still apply -- e.g., don't unnecessarily churn existing code, and follow
> > existing practice within files -- that doesn't mean that checkpatch
> > should be enforcing that for entire directories. Among other reasons,
> > this can cause automatic patch generators to do the exact wrong thing:
> > convert perfectly good existing code into the "networking style", just
> > because it's in a similar directory.
> 
> I believe the patch generator you are referring to is
> checkpatch.

Actually, it was a poor reference to those (people) whose decisions flow
directly from checkpatch (or other code-checking tools) to their
keyboards. It's best not to encourage them, IMO.

Brian

Re: [PATCH] checkpatch: don't encourage new code to use "networking" style comments

[David Miller]

From: Brian Norris <briannorris@chromium.org>
Date: Fri, 28 Apr 2017 12:27:22 -0700

> On Fri, Apr 28, 2017 at 11:24:18AM -0700, Joe Perches wrote:
>> On Fri, 2017-04-28 at 10:55 -0700, Brian Norris wrote:
>> I believe the only person that actually cares about
>> the networking
>> comment style is David Miller.
> 
> Which is why I've CC'd him. If even *he* doesn't care about having
> this warning in checkpatch, then why should anyone else?

Well it potentially saves one round trip for patch submissions.

What I'm not going to do is let people start using different comment
style even for new code in files like net/core/whatever.c after I've
spent nearly two decades getting them to be one way so far.

Re: [PATCH] checkpatch: don't encourage new code to use "networking" style comments

[Brian Norris]

On Fri, Apr 28, 2017 at 03:31:33PM -0400, David Miller wrote:
> From: Brian Norris <briannorris@chromium.org>
> Date: Fri, 28 Apr 2017 12:27:22 -0700
> 
> > On Fri, Apr 28, 2017 at 11:24:18AM -0700, Joe Perches wrote:
> >> On Fri, 2017-04-28 at 10:55 -0700, Brian Norris wrote:
> >> I believe the only person that actually cares about
> >> the networking
> >> comment style is David Miller.
> > 
> > Which is why I've CC'd him. If even *he* doesn't care about having
> > this warning in checkpatch, then why should anyone else?
> 
> Well it potentially saves one round trip for patch submissions.
> 
> What I'm not going to do is let people start using different comment
> style even for new code in files like net/core/whatever.c after I've
> spent nearly two decades getting them to be one way so far.

Well, that sounds like mixed messages to me, but I suppose you're the
(networking) boss.

FWIW, I don't see this consistently applied at all. Unless my regexes
are completely wrong [*], it's roughly 50/50 in drivers/net/ and net/,
and roughly 40/60 (favoring "net" style) in net/core/.

But if that's still the rule, then I guess I'll let this patch drop. And
hope that I'm not the next one Linus notices using the net style and
yells at.

Brian

[*] Which they quite likely are. But here goes:

  Multiline comment with '/*' on first line:
  \/\*$
  Potential (doesn't catch all) multiline comment with '/*' followed by
  text:
  \/\*[^\*][^\*]*$
  FWIW, kerneldoc gets counted for neither.

Re: [PATCH] checkpatch: don't encourage new code to use "networking" style comments

[Joe Perches]

On Fri, 2017-04-28 at 12:51 -0700, Brian Norris wrote:
> FWIW, I don't see this consistently applied at all. Unless my regexes
> are completely wrong [*], it's roughly 50/50 in drivers/net/ and net/,
> and roughly 40/60 (favoring "net" style) in net/core/.

More like 5:1 in net/

$ git grep -E -n "/\*[ \t]*\w" net | grep -v ":1:" | wc -l
30461
$ git grep -E -n "/\*[ \t]*$" net | grep -v ":1:" | wc -l
6061

grep -v ":1:" is to avoid the first line in the file comment

Re: [PATCH] checkpatch: don't encourage new code to use "networking" style comments

[Joe Perches]

On Fri, 2017-04-28 at 12:27 -0700, Brian Norris wrote:
> On Fri, Apr 28, 2017 at 11:24:18AM -0700, Joe Perches wrote:
> > I believe the only person that actually cares about
> > the networking comment style is David Miller.
> 
> Which is why I've CC'd him. If even *he* doesn't care about having this
> warning in checkpatch, then why should anyone else?

Last I looked, David still requests changes to patches
that don't follow that style.