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.