[PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT_STYLE

[PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT_STYLE

[Scott Branden]

NETWORKING_BLOCK_COMMENT_STYLE is supported by checkpatch but there
doesn't seem to be any check for the standard block comment style.
Add support for NONNETWORKING_BLOCK_COMMENT_STYLE to check for empty /*
on first line of non-networking block comments.

Signed-off-by: Scott Branden <scott.branden@broadcom.com>
---
 scripts/checkpatch.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 524df88f9364..899e380782c0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3408,6 +3408,16 @@ sub process {
 			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
 		}
 
+# Non-Networking with an empty initial /*
+		if ($realfile !~ m@^(drivers/net/|net/)@ &&
+		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]/ &&
+		    $prevrawline !~ /\*\/[ \t]*$/ &&		#no trailing */
+		    $rawline =~ /^\+[ \t]*\*/ &&
+		    $realline > 2) {
+			WARN("NONNETWORKING_BLOCK_COMMENT_STYLE",
+			     "non-networking block comments use an empty /* on first line\n" . $hereprev);
+		}
+
 # Block comments use * on subsequent lines
 		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
 		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*
-- 
2.17.1

Re: [PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT_STYLE

[Joe Perches]

On Wed, 2020-06-10 at 13:26 -0700, Scott Branden wrote:
> NETWORKING_BLOCK_COMMENT_STYLE is supported by checkpatch but there
> doesn't seem to be any check for the standard block comment style.
> Add support for NONNETWORKING_BLOCK_COMMENT_STYLE to check for empty /*
> on first line of non-networking block comments.

I think there are _way_ too many instances of this form
in non-networking code to enable this.

$ git grep -P '^\s*/\*\s*\S.*[^\*][^\\]\s*$' -- '*.[ch]' | \
  grep -v -P '^(drivers/net/|net/)' | \
  wc -l
51407

(with a few false positives)

Does anyone really care if non-network code uses
this style?

	/* multiline
	 * comment
	 */

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -3408,6 +3408,16 @@ sub process {
>  			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
>  		}
>  
> +# Non-Networking with an empty initial /*
> +		if ($realfile !~ m@^(drivers/net/|net/)@ &&
> +		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]/ &&
> +		    $prevrawline !~ /\*\/[ \t]*$/ &&		#no trailing */
> +		    $rawline =~ /^\+[ \t]*\*/ &&
> +		    $realline > 2) {
> +			WARN("NONNETWORKING_BLOCK_COMMENT_STYLE",
> +			     "non-networking block comments use an empty /* on first line\n" . $hereprev);
> +		}
> +
>  # Block comments use * on subsequent lines
>  		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
>  		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*

Re: [PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT_STYLE

[Scott Branden]

Hi Joe,

On 2020-06-10 2:16 p.m., Joe Perches wrote:
> On Wed, 2020-06-10 at 13:26 -0700, Scott Branden wrote:
>> NETWORKING_BLOCK_COMMENT_STYLE is supported by checkpatch but there
>> doesn't seem to be any check for the standard block comment style.
>> Add support for NONNETWORKING_BLOCK_COMMENT_STYLE to check for empty /*
>> on first line of non-networking block comments.
> I think there are _way_ too many instances of this form
> in non-networking code to enable this.
>
> $ git grep -P '^\s*/\*\s*\S.*[^\*][^\\]\s*$' -- '*.[ch]' | \
>    grep -v -P '^(drivers/net/|net/)' | \
>    wc -l
> 51407
That is true about many things that checkpatch now checks for that 
didn't previously.
But, by adding to checkpatch the coding style clearly outlined in 
coding-style.rst can be followed:

The preferred style for long (multi-line) comments is:

.. code-block:: c

     /*
      * 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.
      */

For files in net/ and drivers/net/ the preferred style for long (multi-line)
comments is a little different.

.. code-block:: c

     /* The preferred comment style for files in net/ and drivers/net
      * looks like this.
      *
      * It is nearly the same as the generally preferred comment style,
      * but there is no initial almost-blank line.
      */
>
> (with a few false positives)
>
> Does anyone really care if non-network code uses
> this style?
Yes we do.
Consistent coding style is great and keeps your brain able to focus on 
what matters when it is consistent.
>
> 	/* multiline
> 	 * comment
> 	 */
>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -3408,6 +3408,16 @@ sub process {
>>   			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
>>   		}
>>   
>> +# Non-Networking with an empty initial /*
>> +		if ($realfile !~ m@^(drivers/net/|net/)@ &&
>> +		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]/ &&
>> +		    $prevrawline !~ /\*\/[ \t]*$/ &&		#no trailing */
>> +		    $rawline =~ /^\+[ \t]*\*/ &&
>> +		    $realline > 2) {
>> +			WARN("NONNETWORKING_BLOCK_COMMENT_STYLE",
>> +			     "non-networking block comments use an empty /* on first line\n" . $hereprev);
>> +		}
>> +
>>   # Block comments use * on subsequent lines
>>   		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
>>   		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*

Re: [PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT_STYLE

[Joe Perches]

On Wed, 2020-06-10 at 14:33 -0700, Scott Branden wrote:
> Hi Joe,
> 
> On 2020-06-10 2:16 p.m., Joe Perches wrote:
> > On Wed, 2020-06-10 at 13:26 -0700, Scott Branden wrote:
> > > NETWORKING_BLOCK_COMMENT_STYLE is supported by checkpatch but there
> > > doesn't seem to be any check for the standard block comment style.
> > > Add support for NONNETWORKING_BLOCK_COMMENT_STYLE to check for empty /*
> > > on first line of non-networking block comments.
> > I think there are _way_ too many instances of this form
> > in non-networking code to enable this.
> > 
> > $ git grep -P '^\s*/\*\s*\S.*[^\*][^\\]\s*$' -- '*.[ch]' | \
> >    grep -v -P '^(drivers/net/|net/)' | \
> >    wc -l
> > 51407
> That is true about many things that checkpatch now checks for that 
> didn't previously.

Not in that quantity of uses it doesn't.

I specifically did _not_ add this very same test
when I added the other comment tests.

> But, by adding to checkpatch the coding style clearly outlined in 
> coding-style.rst can be followed:

Well, because there are _so_ many false positives
that don't need change, I'm not adding this.

As is, I'm nacking it.

If you need it for your use, you should keep it in
your own tree.

> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -3408,6 +3408,16 @@ sub process {
> > >   			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
> > >   		}
> > >   
> > > +# Non-Networking with an empty initial /*
> > > +		if ($realfile !~ m@^(drivers/net/|net/)@ &&
> > > +		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]/ &&
> > > +		    $prevrawline !~ /\*\/[ \t]*$/ &&		#no trailing */
> > > +		    $rawline =~ /^\+[ \t]*\*/ &&
> > > +		    $realline > 2) {
> > > +			WARN("NONNETWORKING_BLOCK_COMMENT_STYLE",
> > > +			     "non-networking block comments use an empty /* on first line\n" . $hereprev);

_Maybe_ this test _might_ be useful if it did a file/patch
test and used CHK on file, but even then I'm very dubious.

			my $msg_level = \&WARN;
			$msg_level = \&CHK if ($file);
			&{msg_level}(etc...)

Re: [PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT_STYLE

[Scott Branden]

Hi Joe,

On 2020-06-10 2:42 p.m., Joe Perches wrote:
> On Wed, 2020-06-10 at 14:33 -0700, Scott Branden wrote:
>> Hi Joe,
>>
>> On 2020-06-10 2:16 p.m., Joe Perches wrote:
>>> On Wed, 2020-06-10 at 13:26 -0700, Scott Branden wrote:
>>>> NETWORKING_BLOCK_COMMENT_STYLE is supported by checkpatch but there
>>>> doesn't seem to be any check for the standard block comment style.
>>>> Add support for NONNETWORKING_BLOCK_COMMENT_STYLE to check for empty /*
>>>> on first line of non-networking block comments.
>>> I think there are _way_ too many instances of this form
>>> in non-networking code to enable this.
>>>
>>> $ git grep -P '^\s*/\*\s*\S.*[^\*][^\\]\s*$' -- '*.[ch]' | \
>>>     grep -v -P '^(drivers/net/|net/)' | \
>>>     wc -l
>>> 51407
>> That is true about many things that checkpatch now checks for that
>> didn't previously.
> Not in that quantity of uses it doesn't.
>
> I specifically did _not_ add this very same test
> when I added the other comment tests.
>
>> But, by adding to checkpatch the coding style clearly outlined in
>> coding-style.rst can be followed:
> Well, because there are _so_ many false positives
> that don't need change, I'm not adding this.
>
> As is, I'm nacking it.
>
> If you need it for your use, you should keep it in
> your own tree.
I think this has value for others.
>
>>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>>> []
>>>> @@ -3408,6 +3408,16 @@ sub process {
>>>>    			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
>>>>    		}
>>>>    
>>>> +# Non-Networking with an empty initial /*
>>>> +		if ($realfile !~ m@^(drivers/net/|net/)@ &&
>>>> +		    $prevrawline =~ /^\+[ \t]*\/\*[ \t]/ &&
>>>> +		    $prevrawline !~ /\*\/[ \t]*$/ &&		#no trailing */
>>>> +		    $rawline =~ /^\+[ \t]*\*/ &&
>>>> +		    $realline > 2) {
>>>> +			WARN("NONNETWORKING_BLOCK_COMMENT_STYLE",
>>>> +			     "non-networking block comments use an empty /* on first line\n" . $hereprev);
> _Maybe_ this test _might_ be useful if it did a file/patch
> test and used CHK on file, but even then I'm very dubious.
>
> 			my $msg_level = \&WARN;
> 			$msg_level = \&CHK if ($file);
> 			&{msg_level}(etc...)
I can make such change.
As is, checkpatch basically follows the coding style documented for 
networking and has no check for outside.

I do not know the history of the 2 block coding styles but here's a 
radical thought:
Change the coding-style document to pick a single coding style for ALL 
block comment styles and
use that going forward in checkpatch instead?
>
>

Re: [PATCH] checkpatch: add check for NONNETWORKING_BLOCK_COMMENT_STYLE

[Joe Perches]

On Wed, 2020-06-10 at 15:08 -0700, Scott Branden wrote:
> Hi Joe,

Hi Scott.

> I do not know the history of the 2 block coding styles but here's a 
> radical thought:
> Change the coding-style document to pick a single coding style for ALL 
> block comment styles and
> use that going forward in checkpatch instead?

Opinions vary.
Good luck with that.
Seriously.