Spurious warning in checkpatch.pl?

Spurious warning in checkpatch.pl?

[Larry Finger]

Andy,

The checkpatch.pl version in mainline is issuing what I think are false warnings 
of the type

WARNING: networking block comments put the trailing */ on a separate line
#93: FILE: drivers/net/wireless/rtlwifi/wifi.h:208:
+       u32 rf_rb;              /* rflssi_readback */

Is a trailing comment for the member of a struct not allowed?

Thanks,

Larry

Re: Spurious warning in checkpatch.pl?

[Joe Perches]

On Fri, 2012-10-12 at 13:23 -0500, Larry Finger wrote:
> Andy,
> 
> The checkpatch.pl version in mainline is issuing what I think are false warnings 
> of the type
> 
> WARNING: networking block comments put the trailing */ on a separate line
> #93: FILE: drivers/net/wireless/rtlwifi/wifi.h:208:
> +       u32 rf_rb;              /* rflssi_readback */
> 
> Is a trailing comment for the member of a struct not allowed?

It's allowed.  Maybe even better, encouraged.

It's a change I made in commit 058806007450
("checkpatch: check networking specific block comment style")
without sufficient testing.

I'll see about fixing it.

Re: Spurious warning in checkpatch.pl?

[Larry Finger]

On 10/12/2012 01:38 PM, Joe Perches wrote:
> On Fri, 2012-10-12 at 13:23 -0500, Larry Finger wrote:
>> Andy,
>>
>> The checkpatch.pl version in mainline is issuing what I think are false warnings
>> of the type
>>
>> WARNING: networking block comments put the trailing */ on a separate line
>> #93: FILE: drivers/net/wireless/rtlwifi/wifi.h:208:
>> +       u32 rf_rb;              /* rflssi_readback */
>>
>> Is a trailing comment for the member of a struct not allowed?
>
> It's allowed.  Maybe even better, encouraged.
>
> It's a change I made in commit 058806007450
> ("checkpatch: check networking specific block comment style")
> without sufficient testing.
>
> I'll see about fixing it.

Joe,

Good. I will ignore those warnings.

One more question. Was the warning below intended?

WARNING: networking block comments put the trailing */ on a separate line
#1846: FILE: drivers/net/wireless/rtlwifi/rtl8723ae/hal_btc.h:27:
+ *****************************************************************************/

To me, that line at the end of a copyright block looks better then


  ****************************************************************************
  */

Larry

Re: Spurious warning in checkpatch.pl?

[Joe Perches]

On Fri, 2012-10-12 at 14:49 -0500, Larry Finger wrote:
> One more question. Was the warning below intended?
> 
> WARNING: networking block comments put the trailing */ on a separate line
> #1846: FILE: drivers/net/wireless/rtlwifi/rtl8723ae/hal_btc.h:27:
> + *****************************************************************************/
> 
> To me, that line at the end of a copyright block looks better then
> 
> 
>   ****************************************************************************
>   */

No, not really.  It's a side effect of the nominal block comment
style that networking uses.

Maybe it could be improved by ignoring lines that end in "**/"
(\*\*\/\s*$) too.

[PATCH] checkpatch: Improve network block comment style checking

[Joe Perches]

Some comment styles in net and drivers/net are flagged inappropriately.

Avoid proclaiming inline comments like:
	int a = b;	/* some comment */
and block comments like:
	/*********************
	 * some comment
	 ********************/
are defective.

Tested with
$ cat drivers/net/t.c
/* foo */

/*
 * foo
 */

/* foo
 */

/* foo
 * bar */

/****************************
 * some long block comment
 ***************************/

struct foo {
	int bar;	/* another test */
};
$

Reported-by: Larry Finger <Larry.Finger@lwfinger.net>
Signed-off-by: Joe Perches <joe@perches.com>
---
 scripts/checkpatch.pl |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 21a9f5d..f18750e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1890,8 +1890,10 @@ sub process {
 		}
 
 		if ($realfile =~ m@^(drivers/net/|net/)@ &&
-		    $rawline !~ m@^\+[ \t]*(\/\*|\*\/)@ &&
-		    $rawline =~ m@^\+[ \t]*.+\*\/[ \t]*$@) {
+		    $rawline !~ m@^\+[ \t]*\*/[ \t]*$@ &&	#trailing */
+		    $rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&	#inline /*...*/
+		    $rawline !~ m@^\+.*\*{2,}/[ \t]*$@ &&	#trailing **/
+		    $rawline =~ m@^\+[ \t]*.+\*\/[ \t]*$@) {	#non blank */
 			WARN("NETWORKING_BLOCK_COMMENT_STYLE",
 			     "networking block comments put the trailing */ on a separate line\n" . $herecurr);
 		}