[PATCH v1] checkpatch: test missing initial blank line in block comment

[PATCH v1] checkpatch: test missing initial blank line in block comment

[Hugues Fruchet]

Hi checkpatch maintainers,

here is a patch tentative to raise a warning when multiple line block comments
are not starting with empty blank comment:

Warn when block comments are not starting with blank comment:
    /* multiple lines
     * block comment,
     * => warning
     */

    /*
     * multiple lines
     * block comment,
     * => no warning
     */

Exception made for networking files where rule is the
exact opposite.


* sample output:

$ scripts/checkpatch.pl -f drivers/media/v4l2-core/* | grep empty -A2
WARNING: Block comments starts with an empty /*
#393: FILE: drivers/media/v4l2-core/v4l2-compat-ioctl32.c:393:
+	/* For MMAP, driver might've set up the offset, so copy it back.
--
WARNING: Block comments starts with an empty /*
#661: FILE: drivers/media/v4l2-core/v4l2-ctrls.c:661:
+	/* The MPEG controls are applicable to all codec controls
--
+	/* Add immediately at the end of the list if the list is empty, or if
+	   the last element in the list has a lower ID.
[...]


* sample output in drivers/net/ (non-regression check):

$ scripts/checkpatch.pl -f drivers/net/*/* | grep empty -A2
WARNING: networking block comments don't use an empty /* line, use /* Comment...
#39: FILE: drivers/net/appletalk/cops.c:39:
+/*
--
WARNING: networking block comments don't use an empty /* line, use /* Comment...
#45: FILE: drivers/net/appletalk/cops.c:45:
+/*
--
[...]

Hugues Fruchet (1):
  checkpatch: test missing initial blank line in block comment

 scripts/checkpatch.pl | 11 +++++++++++
 1 file changed, 11 insertions(+)

-- 
1.9.1

[PATCH v1] checkpatch: test missing initial blank line in block comment

[Hugues Fruchet]

Warn when block comments are not starting with blank comment:

/* multiple lines
 * block comment,
 * => warning
 */

/*
 * multiple lines
 * block comment,
 * => no warning
 */

Exception made for networking files where rule is the
exact opposite.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 scripts/checkpatch.pl | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index baa3c7b..8754c9d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3000,6 +3000,17 @@ sub process {
 			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
 		}
 
+# Block comment styles
+# Missing initial /*
+		if ($realfile !~ m@^(drivers/net/|net/)@ &&	#networking exception
+		    $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&	#start with /*...
+		    $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&	#no inline /*...*/
+		    $rawline =~ /^\+[ \t]*\*/ &&
+		    $realline > 2) {
+			WARN("BLOCK_COMMENT_STYLE",
+			     "Block comments starts with an empty /*\n" . $hereprev);
+		}
+
 # Block comments use * on subsequent lines
 		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
 		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*
-- 
1.9.1

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Joe Perches]

On Mon, 2017-04-03 at 10:08 +0200, Hugues Fruchet wrote:
> Warn when block comments are not starting with blank comment:
> 
> /* multiple lines
>  * block comment,
>  * => warning
>  */
> 
> /*
>  * multiple lines
>  * block comment,
>  * => no warning
>  */
> 
> Exception made for networking files where rule is the
> exact opposite.

I recall there was some reason I didn't do this
when adding the block comment code, but I don't
recall what it was.  Perhaps it was the initial
line of files.

Maybe your $realline > 2 test fixes it.  Maybe not.
Dunno.

If you run this against the entire kernel code
using a unique test type and not BLOCK_COMMENT_STYLE
are there any false positives?

Maybe test with something like:

$ git ls-files -- "*.[ch]" | \
  xargs --max-args 20 ./scripts/checkpatch.pl -f --types=<your_unique_test>

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Hugues FRUCHET]

Hi Joe, thanks for reviewing,

I have run the command you advice on the entire kernel code, modifying 
the script to only match the newly introduced check case.
There was 14389 hits, quite huge, so I cannot 100% certify that there 
are no false positives, but I have checked the output carefully and 
found 2 limit cases:

1) space character placed just after "/*"
WARNING: Block comments starts with an empty /*
#330: FILE: arch/alpha/kernel/core_irongate.c:330:
+	/*
+	 * Check for within the AGP aperture...
=> 146 hits (grep -c -n -E "\/\* $" /tmp/check.txt)

2) // style comment followed by pointer dereference
WARNING: Block comments starts with an empty /*
#426: FILE: drivers/media/dvb-core/dvb_ca_en50221.c:426:
+	// success
+	*tupleType = _tupleType;
=> 4 hits

Anyway this reveal comment style related issues, so I would say that we 
can keep script as it is, what do you think about ?

Here is the count detail by first level of directories within kernel:

$ grep FILE /tmp/check.txt | grep -c "FILE: drivers/"
8859
$ grep FILE /tmp/check.txt | grep -c "FILE: arch/"
2306
$ grep FILE /tmp/check.txt | grep -c "FILE: fs/"
1136
$ grep FILE /tmp/check.txt | grep -c "FILE: sound/"
810
$ grep FILE /tmp/check.txt | grep -c "FILE: include/"
669
$ grep FILE /tmp/check.txt | grep -c "FILE: kernel/"
143
$ grep FILE /tmp/check.txt | grep -c "FILE: security/"
112
$ grep FILE /tmp/check.txt | grep -c "FILE: lib/"
91
$ grep FILE /tmp/check.txt | grep -c "FILE: tools/"
81
$ grep FILE /tmp/check.txt | grep -c "FILE: crypto/"
54
$ grep FILE /tmp/check.txt | grep -c "FILE: scripts/"
44
$ grep FILE /tmp/check.txt | grep -c "FILE: mm/"
35
$ grep FILE /tmp/check.txt | grep -c "FILE: block/"
27
$ grep FILE /tmp/check.txt | grep -c "FILE: virt/"
8
$ grep FILE /tmp/check.txt | grep -c "FILE: samples/"
5
$ grep FILE /tmp/check.txt | grep -c "FILE: ipc/"
5
$ grep FILE /tmp/check.txt | grep -c "FILE: certs/"
1

The complete output is there for reference: 
http://paste.ubuntu.com/24319042/


Best regards,
Hugues.

On 04/03/2017 09:06 PM, Joe Perches wrote:
> On Mon, 2017-04-03 at 10:08 +0200, Hugues Fruchet wrote:
>> Warn when block comments are not starting with blank comment:
>>
>> /* multiple lines
>>  * block comment,
>>  * => warning
>>  */
>>
>> /*
>>  * multiple lines
>>  * block comment,
>>  * => no warning
>>  */
>>
>> Exception made for networking files where rule is the
>> exact opposite.
>
> I recall there was some reason I didn't do this
> when adding the block comment code, but I don't
> recall what it was.  Perhaps it was the initial
> line of files.
>
> Maybe your $realline > 2 test fixes it.  Maybe not.
> Dunno.
>
> If you run this against the entire kernel code
> using a unique test type and not BLOCK_COMMENT_STYLE
> are there any false positives?
>
> Maybe test with something like:
>
> $ git ls-files -- "*.[ch]" | \
>   xargs --max-args 20 ./scripts/checkpatch.pl -f --types=<your_unique_test>
>

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Joe Perches]

On Wed, 2017-04-05 at 08:23 +0000, Hugues FRUCHET wrote:
> Hi Joe, thanks for reviewing,

Hello Hugues

> I have run the command you advice on the entire kernel code, modifying 
> the script to only match the newly introduced check case.
> There was 14389 hits, quite huge, so I cannot 100% certify that there 
> are no false positives, but I have checked the output carefully and 
> found 2 limit cases:
> 
> 1) space character placed just after "/*"
> WARNING: Block comments starts with an empty /*
> #330: FILE: arch/alpha/kernel/core_irongate.c:330:
> +	/*
> +	 * Check for within the AGP aperture...
> => 146 hits (grep -c -n -E "\/\* $" /tmp/check.txt)
> 
> 2) // style comment followed by pointer dereference
> WARNING: Block comments starts with an empty /*
> #426: FILE: drivers/media/dvb-core/dvb_ca_en50221.c:426:
> +	// success
> +	*tupleType = _tupleType;
> => 4 hits
> 
> Anyway this reveal comment style related issues, so I would say that we 
> can keep script as it is, what do you think about ?

Glancing at the output, there is also the comment
in a multiline macro case:

WARNING: Block comments starts with an empty /*
#354: FILE: arch/mips/include/asm/processor.h:354:
+	/*							\
+	 * Other stuff associated with the process		\

Dunno how common that is, but maybe the test
should be changed to avoid those.

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Hugues FRUCHET]

On 04/05/2017 10:35 AM, Joe Perches wrote:
> On Wed, 2017-04-05 at 08:23 +0000, Hugues FRUCHET wrote:
>> Hi Joe, thanks for reviewing,
>
> Hello Hugues
>
>> I have run the command you advice on the entire kernel code, modifying
>> the script to only match the newly introduced check case.
>> There was 14389 hits, quite huge, so I cannot 100% certify that there
>> are no false positives, but I have checked the output carefully and
>> found 2 limit cases:
>>
>> 1) space character placed just after "/*"
>> WARNING: Block comments starts with an empty /*
>> #330: FILE: arch/alpha/kernel/core_irongate.c:330:
>> +	/*
>> +	 * Check for within the AGP aperture...
>> => 146 hits (grep -c -n -E "\/\* $" /tmp/check.txt)
>>
>> 2) // style comment followed by pointer dereference
>> WARNING: Block comments starts with an empty /*
>> #426: FILE: drivers/media/dvb-core/dvb_ca_en50221.c:426:
>> +	// success
>> +	*tupleType = _tupleType;
>> => 4 hits
>>
>> Anyway this reveal comment style related issues, so I would say that we
>> can keep script as it is, what do you think about ?
>
> Glancing at the output, there is also the comment
> in a multiline macro case:
>
> WARNING: Block comments starts with an empty /*
> #354: FILE: arch/mips/include/asm/processor.h:354:
> +	/*							\
> +	 * Other stuff associated with the process		\
>
> Dunno how common that is, but maybe the test
> should be changed to avoid those.
>

Here is a proposal that remove this macro case:

# Missing initial /*
if ($realfile !~ m@^(drivers/net/|net/)@ &&	#networking exception
     $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&	#start with /*...
     $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&	#no inline /*...*/
+   $prevrawline !~ /^\+[ \t]\/\*+[ \t]+\\$/ &&#no macro /*<tab><\>
     $rawline =~ /^\+[ \t]*\*/ &&
     $realline > 2) {
	WARN("MISSING_INITIAL_BLOCK_COMMENT_STYLE",
	     "Block comments starts with an empty /*\n" . $hereprev);
}

BR,
Hugues.

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Joe Perches]

On Wed, 2017-04-05 at 09:43 +0000, Hugues FRUCHET wrote:
> 
> On 04/05/2017 10:35 AM, Joe Perches wrote:
> > On Wed, 2017-04-05 at 08:23 +0000, Hugues FRUCHET wrote:
> > > Hi Joe, thanks for reviewing,
> > 
> > Hello Hugues
> > 
> > > I have run the command you advice on the entire kernel code, modifying
> > > the script to only match the newly introduced check case.
> > > There was 14389 hits, quite huge, so I cannot 100% certify that there
> > > are no false positives, but I have checked the output carefully and
> > > found 2 limit cases:
> > > 
> > > 1) space character placed just after "/*"
> > > WARNING: Block comments starts with an empty /*
> > > #330: FILE: arch/alpha/kernel/core_irongate.c:330:
> > > +	/*
> > > +	 * Check for within the AGP aperture...
> > > => 146 hits (grep -c -n -E "\/\* $" /tmp/check.txt)
> > > 
> > > 2) // style comment followed by pointer dereference
> > > WARNING: Block comments starts with an empty /*
> > > #426: FILE: drivers/media/dvb-core/dvb_ca_en50221.c:426:
> > > +	// success
> > > +	*tupleType = _tupleType;
> > > => 4 hits
> > > 
> > > Anyway this reveal comment style related issues, so I would say that we
> > > can keep script as it is, what do you think about ?
> > 
> > Glancing at the output, there is also the comment
> > in a multiline macro case:
> > 
> > WARNING: Block comments starts with an empty /*
> > #354: FILE: arch/mips/include/asm/processor.h:354:
> > +	/*							\
> > +	 * Other stuff associated with the process		\
> > 
> > Dunno how common that is, but maybe the test
> > should be changed to avoid those.
> > 
> 
> Here is a proposal that remove this macro case:Per
> 
> # Missing initial /*
> if ($realfile !~ m@^(drivers/net/|net/)@ &&	#networking exception
>      $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&	#start with /*...
>      $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&	#no inline /*...*/
> +   $prevrawline !~ /^\+[ \t]\/\*+[ \t]+\\$/ &&#no macro /*<tab><\>
>      $rawline =~ /^\+[ \t]*\*/ &&
>      $realline > 2) {

Perhaps it's better to change this to 

	$prevrawline !~ /^\+\s*\/\*.*\\$/

Also perhaps the
	// foo
	*bar = baz;

case could be avoided by adding tests for the
comment character $; on $prevline and $line
and not looking only at $prevrawline and $rawline.

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Hugues FRUCHET]

On 04/05/2017 11:55 AM, Joe Perches wrote:
> On Wed, 2017-04-05 at 09:43 +0000, Hugues FRUCHET wrote:
>>
>> On 04/05/2017 10:35 AM, Joe Perches wrote:
>>> On Wed, 2017-04-05 at 08:23 +0000, Hugues FRUCHET wrote:
>>>> Hi Joe, thanks for reviewing,
>>>
>>> Hello Hugues
>>>
>>>> I have run the command you advice on the entire kernel code, modifying
>>>> the script to only match the newly introduced check case.
>>>> There was 14389 hits, quite huge, so I cannot 100% certify that there
>>>> are no false positives, but I have checked the output carefully and
>>>> found 2 limit cases:
>>>>
>>>> 1) space character placed just after "/*"
>>>> WARNING: Block comments starts with an empty /*
>>>> #330: FILE: arch/alpha/kernel/core_irongate.c:330:
>>>> +	/*
>>>> +	 * Check for within the AGP aperture...
>>>> => 146 hits (grep -c -n -E "\/\* $" /tmp/check.txt)
>>>>
>>>> 2) // style comment followed by pointer dereference
>>>> WARNING: Block comments starts with an empty /*
>>>> #426: FILE: drivers/media/dvb-core/dvb_ca_en50221.c:426:
>>>> +	// success
>>>> +	*tupleType = _tupleType;
>>>> => 4 hits
>>>>
>>>> Anyway this reveal comment style related issues, so I would say that we
>>>> can keep script as it is, what do you think about ?
>>>
>>> Glancing at the output, there is also the comment
>>> in a multiline macro case:
>>>
>>> WARNING: Block comments starts with an empty /*
>>> #354: FILE: arch/mips/include/asm/processor.h:354:
>>> +	/*							\
>>> +	 * Other stuff associated with the process		\
>>>
>>> Dunno how common that is, but maybe the test
>>> should be changed to avoid those.
>>>
>>
>> Here is a proposal that remove this macro case:Per
>>
>> # Missing initial /*
>> if ($realfile !~ m@^(drivers/net/|net/)@ &&	#networking exception
>>      $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&	#start with /*...
>>      $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&	#no inline /*...*/
>> +   $prevrawline !~ /^\+[ \t]\/\*+[ \t]+\\$/ &&#no macro /*<tab><\>
>>      $rawline =~ /^\+[ \t]*\*/ &&
>>      $realline > 2) {
>
> Perhaps it's better to change this to
>
> 	$prevrawline !~ /^\+\s*\/\*.*\\$/

KO with this line, I suspect you meant "\s" instead of "." in above 
expression, so I've changed to:
        $prevrawline !~ /^\+\s*\/\*\s*\\$/
this one is OK

>
> Also perhaps the
> 	// foo
> 	*bar = baz;
>
> case could be avoided by adding tests for the
> comment character $; on $prevline and $line
> and not looking only at $prevrawline and $rawline.
>

Sorry for my poor understanding of the script but I don't catch what you 
meant regarding "raw" and non "raw" variables, so I've done the job 
simply by excluding the lines starting with "//":
       $prevrawline !~ /^\+.*\/\/.*[ \t]*/ &&	#no inline //

Which gives finally:

# Missing initial /*
if ($realfile !~ m@^(drivers/net/|net/)@ &&	#networking exception
   $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&	#start with /*...
   $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&	#no inline /*...*/
+ $prevrawline !~ /^\+.*\/\/.*[ \t]*/ &&	#no inline //
+ $prevrawline !~ /^\+\s*\/\*\s*\\$/ &&	#no macro /*<whitespace><\>
   $rawline =~ /^\+[ \t]*\*/ &&
   $realline > 2) {
  	WARN("MISSING_INITIAL_BLOCK_COMMENT_STYLE",
  	     "Block comments starts with an empty /*\n" . $hereprev);
     }


BR,
Hugues.

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Hugues FRUCHET]

Hi Joe,

here is the output with the last version of the script: 
https://paste.ubuntu.com/24333124/

Differences are on the macro cases and the //foo \ *bar, no more warned.

BR,
Hugues.

On 04/05/2017 03:26 PM, Hugues Fruchet wrote:
>
>
> On 04/05/2017 11:55 AM, Joe Perches wrote:
>> On Wed, 2017-04-05 at 09:43 +0000, Hugues FRUCHET wrote:
>>>
>>> On 04/05/2017 10:35 AM, Joe Perches wrote:
>>>> On Wed, 2017-04-05 at 08:23 +0000, Hugues FRUCHET wrote:
>>>>> Hi Joe, thanks for reviewing,
>>>>
>>>> Hello Hugues
>>>>
>>>>> I have run the command you advice on the entire kernel code, modifying
>>>>> the script to only match the newly introduced check case.
>>>>> There was 14389 hits, quite huge, so I cannot 100% certify that there
>>>>> are no false positives, but I have checked the output carefully and
>>>>> found 2 limit cases:
>>>>>
>>>>> 1) space character placed just after "/*"
>>>>> WARNING: Block comments starts with an empty /*
>>>>> #330: FILE: arch/alpha/kernel/core_irongate.c:330:
>>>>> +    /*
>>>>> +     * Check for within the AGP aperture...
>>>>> => 146 hits (grep -c -n -E "\/\* $" /tmp/check.txt)
>>>>>
>>>>> 2) // style comment followed by pointer dereference
>>>>> WARNING: Block comments starts with an empty /*
>>>>> #426: FILE: drivers/media/dvb-core/dvb_ca_en50221.c:426:
>>>>> +    // success
>>>>> +    *tupleType = _tupleType;
>>>>> => 4 hits
>>>>>
>>>>> Anyway this reveal comment style related issues, so I would say
>>>>> that we
>>>>> can keep script as it is, what do you think about ?
>>>>
>>>> Glancing at the output, there is also the comment
>>>> in a multiline macro case:
>>>>
>>>> WARNING: Block comments starts with an empty /*
>>>> #354: FILE: arch/mips/include/asm/processor.h:354:
>>>> +    /*                            \
>>>> +     * Other stuff associated with the process        \
>>>>
>>>> Dunno how common that is, but maybe the test
>>>> should be changed to avoid those.
>>>>
>>>
>>> Here is a proposal that remove this macro case:Per
>>>
>>> # Missing initial /*
>>> if ($realfile !~ m@^(drivers/net/|net/)@ &&    #networking exception
>>>      $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&    #start with /*...
>>>      $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&    #no inline /*...*/
>>> +   $prevrawline !~ /^\+[ \t]\/\*+[ \t]+\\$/ &&#no macro /*<tab><\>
>>>      $rawline =~ /^\+[ \t]*\*/ &&
>>>      $realline > 2) {
>>
>> Perhaps it's better to change this to
>>
>>     $prevrawline !~ /^\+\s*\/\*.*\\$/
>
> KO with this line, I suspect you meant "\s" instead of "." in above
> expression, so I've changed to:
>        $prevrawline !~ /^\+\s*\/\*\s*\\$/
> this one is OK
>
>>
>> Also perhaps the
>>     // foo
>>     *bar = baz;
>>
>> case could be avoided by adding tests for the
>> comment character $; on $prevline and $line
>> and not looking only at $prevrawline and $rawline.
>>
>
> Sorry for my poor understanding of the script but I don't catch what you
> meant regarding "raw" and non "raw" variables, so I've done the job
> simply by excluding the lines starting with "//":
>       $prevrawline !~ /^\+.*\/\/.*[ \t]*/ &&    #no inline //
>
> Which gives finally:
>
> # Missing initial /*
> if ($realfile !~ m@^(drivers/net/|net/)@ &&    #networking exception
>   $prevrawline =~ /^\+[ \t]\/\**.+[ \t]/ &&    #start with /*...
>   $prevrawline !~ /^\+.*\/\*.*\*\/[ \t]*/ &&    #no inline /*...*/
> + $prevrawline !~ /^\+.*\/\/.*[ \t]*/ &&    #no inline //
> + $prevrawline !~ /^\+\s*\/\*\s*\\$/ &&    #no macro /*<whitespace><\>
>   $rawline =~ /^\+[ \t]*\*/ &&
>   $realline > 2) {
>      WARN("MISSING_INITIAL_BLOCK_COMMENT_STYLE",
>           "Block comments starts with an empty /*\n" . $hereprev);
>     }
>
>
> BR,
> Hugues.

Re: [PATCH v1] checkpatch: test missing initial blank line in block comment

[Joe Perches]

On Fri, 2017-04-07 at 09:56 +0000, Hugues FRUCHET wrote:
> Hi Joe,

Hi again Hugues.

> here is the output with the last version of the script: 
> https://paste.ubuntu.com/24333124/
> 
> Differences are on the macro cases and the //foo \ *bar, no more warned.

Thanks.

I guess my only real concern about this test is
there are ~15000 instances of this in the tree.

Do maintainers care if comments are formatted

	/*
	 * [multiple...]
	 * line comment
	 */

vs

	/* [multiple...]
	 * line comment
	 */

enough to want others to submit patches
changing from the latter style?

The reason the networking checking exists is
because David Miller, the primary networking
maintainer, was constantly telling others to
resubmit patches to his preferred style.

I doubt there's another maintainer that cares
that much one way or another.

I don't.

Any opinions from anyone else?