Patchwork checkpatch.pl: Fix warnings on code comments

login
register
mail settings
Submitter Jeff Kirsher
Date Jan. 27, 2013, 11:35 a.m.
Message ID <1359286539-18395-1-git-send-email-jeffrey.t.kirsher@intel.com>
Download mbox | patch
Permalink /patch/215986/
State Rejected
Delegated to: David Miller
Headers show

Comments

Jeff Kirsher - Jan. 27, 2013, 11:35 a.m.
The following commit:
commit 058806007450489bb8f457b275e5cb5c946320c1
Author: Joe Perches <joe@perches.com>
Date:   Thu Oct 4 17:13:35 2012 -0700

checkpatch: check networking specific block comment style

Produces warnings on code comments which follow the Linux coding style
guide.  While the desired code comment style for networking my differ
from the rest of the kernel, both styles should be permitted.

This patch reverts a portion of the commit to allow multi-line code
comments to use either style.

Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Tested-by: Jeff Pieper <jeffrey.e.pieper@intel.com>
---
 scripts/checkpatch.pl | 7 -------
 1 file changed, 7 deletions(-)
Joe Perches - Jan. 27, 2013, 5:11 p.m.
On Sun, 2013-01-27 at 03:35 -0800, Jeff Kirsher wrote:
> This patch reverts a portion of the commit to allow multi-line code
> comments to use either style.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -1878,13 +1878,6 @@ sub process {
>  		}
>  
>  		if ($realfile =~ m@^(drivers/net/|net/)@ &&
> -		    $rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
> -		    $prevrawline =~ /^\+[ \t]*$/) {
> -			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
> -			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
> -		}
> -
> -		if ($realfile =~ m@^(drivers/net/|net/)@ &&
>  		    $rawline !~ m@^\+[ \t]*\*/[ \t]*$@ &&	#trailing */
>  		    $rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&	#inline /*...*/
>  		    $rawline !~ m@^\+.*\*{2,}/[ \t]*$@ &&	#trailing **/

Shrug.

Ignore things you don't agree with.

David has written many, many emails
requesting changes for this specific
comment style.

I think you and Intel should use
--ignore=NETWORKING_BLOCK_COMMENT_STYLE
to avoid seeing this if you don't agree
with it.

I also think expectations that everyone
will agree with every checkpatch bleat
are unrealistic.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - Jan. 27, 2013, 11:59 p.m.
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Sun, 27 Jan 2013 03:35:39 -0800

> Produces warnings on code comments which follow the Linux coding style
> guide.  While the desired code comment style for networking my differ
> from the rest of the kernel, both styles should be permitted.

I was actually going to mention to you guys that I've been lackadasical
about enforcing the comment style I want with the Intel drivers.

That was a mistake, I should have enforced it strictly, as I do for
the other drivers and the core networking code, from the beginning.

And it's clearly a mistake if you feel the need to take out the very
checkpatch working that's meant to enforce this comment style in all
of the networking drivers and core.

Do not revert this, follow it's advice instead.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Kirsher - Jan. 28, 2013, 2:56 a.m.
On Sun, 2013-01-27 at 18:59 -0500, David Miller wrote:
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Sun, 27 Jan 2013 03:35:39 -0800
> 
> > Produces warnings on code comments which follow the Linux coding style
> > guide.  While the desired code comment style for networking my differ
> > from the rest of the kernel, both styles should be permitted.
> 
> I was actually going to mention to you guys that I've been lackadasical
> about enforcing the comment style I want with the Intel drivers.
> 
> That was a mistake, I should have enforced it strictly, as I do for
> the other drivers and the core networking code, from the beginning.
> 
> And it's clearly a mistake if you feel the need to take out the very
> checkpatch working that's meant to enforce this comment style in all
> of the networking drivers and core.
> 
> Do not revert this, follow it's advice instead.

Ok, I am fine with that.  I just had not seen any emails/responses that
this was direction you wanted to go.

So will you be fine with cleanup patches which go through and convert
all the existing code comments to the desired format?  If so, I will get
started on patches to cleanup,convert the Intel drivers to the desired
code comment style.
David Miller - Jan. 28, 2013, 3:07 a.m.
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Sun, 27 Jan 2013 18:56:45 -0800

> So will you be fine with cleanup patches which go through and
> convert all the existing code comments to the desired format?

Sure.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Allan, Bruce W - Jan. 28, 2013, 5:17 p.m.
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of David Miller
> Sent: Sunday, January 27, 2013 7:07 PM
> To: Kirsher, Jeffrey T
> Cc: apw@canonical.com; linux-kernel@vger.kernel.org;
> netdev@vger.kernel.org
> Subject: Re: [PATCH] checkpatch.pl: Fix warnings on code comments
> 
> From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> Date: Sun, 27 Jan 2013 18:56:45 -0800
> 
> > So will you be fine with cleanup patches which go through and
> > convert all the existing code comments to the desired format?
> 
> Sure.

Not all Intel drivers...e1000e already conforms to the comment style :-)

Bruce.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Joe Perches - Jan. 28, 2013, 5:30 p.m.
On Mon, 2013-01-28 at 17:17 +0000, Allan, Bruce W wrote:
> David Miller Sent: Sunday, January 27, 2013 7:07 PM
> > From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > So will you be fine with cleanup patches which go through and
> > > convert all the existing code comments to the desired format?
> > Sure.
> Not all Intel drivers...e1000e already conforms to the comment style :-)

In case anyone cares, here's a perl regex
to do network comment style conversion.

	$text =~ s@/\*[ \t]*\n[ \t]*\*@/*@g;
	$text =~ s@/\*([ \t]*)([^\n]+)\n[ \t]*\*/@/\*$1$2 \*/@g;

(assumes the entire file is in $text)

It creates a ~220KB diff for drivers/net/ethernet/intel/
that I won't post.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jeff Kirsher - Jan. 28, 2013, 8:56 p.m.
On Mon, 2013-01-28 at 09:30 -0800, Joe Perches wrote:
> On Mon, 2013-01-28 at 17:17 +0000, Allan, Bruce W wrote:
> > David Miller Sent: Sunday, January 27, 2013 7:07 PM
> > > From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> > > > So will you be fine with cleanup patches which go through and
> > > > convert all the existing code comments to the desired format?
> > > Sure.
> > Not all Intel drivers...e1000e already conforms to the comment style :-)
> 
> In case anyone cares, here's a perl regex
> to do network comment style conversion.
> 
>         $text =~ s@/\*[ \t]*\n[ \t]*\*@/*@g;
>         $text =~ s@/\*([ \t]*)([^\n]+)\n[ \t]*\*/@/\*$1$2 \*/@g;
> 
> (assumes the entire file is in $text)
> 
> It creates a ~220KB diff for drivers/net/ethernet/intel/
> that I won't post.
> 

Thanks Joe, I will get patches to take care of the Intel drivers (minus
e1000e since Bruce has already completed that work).

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4d2c7df..d3ffec5 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1878,13 +1878,6 @@  sub process {
 		}
 
 		if ($realfile =~ m@^(drivers/net/|net/)@ &&
-		    $rawline =~ /^\+[ \t]*\/\*[ \t]*$/ &&
-		    $prevrawline =~ /^\+[ \t]*$/) {
-			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
-			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
-		}
-
-		if ($realfile =~ m@^(drivers/net/|net/)@ &&
 		    $rawline !~ m@^\+[ \t]*\*/[ \t]*$@ &&	#trailing */
 		    $rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&	#inline /*...*/
 		    $rawline !~ m@^\+.*\*{2,}/[ \t]*$@ &&	#trailing **/