Message-ID: <20230927081959.210166e9@meshulam.tesarici.cz>
Date:   Wed, 27 Sep 2023 08:19:59 +0200
From:   Petr Tesařík <petr@...arici.cz>
To:     Joe Perches <joe@...ches.com>
Cc:     Andy Whitcroft <apw@...onical.com>,
        Dwaipayan Ray <dwaipayanray1@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>,
        open list <linux-kernel@...r.kernel.org>,
        Catalin Marinas <catalin.marinas@....com>
Subject: Re: [PATCH v2] checkpatch: warn about multi-line comments without
 an empty /* line

Hi Joe,

On Tue, 26 Sep 2023 13:39:34 -0700
Joe Perches <joe@...ches.com> wrote:

> On Tue, 2023-09-26 at 22:16 +0200, Petr Tesařík wrote:
> > On Tue, 26 Sep 2023 12:56:33 -0700
> > Joe Perches <joe@...ches.com> wrote:
> >   
> > > On Tue, 2023-09-26 at 21:24 +0200, Petr Tesarik wrote:  
> > > > According to Documentation/process/coding-style.rst, the preferred style
> > > > for multi-line comments outside net/ and drivers/net/ 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.
> > > >          */
> > > > 
> > > > Signed-off-by: Petr Tesarik <petr@...arici.cz>    
> > > []  
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl    
> > > []  
> > > > @@ -4006,6 +4006,14 @@ sub process {
> > > >  			     "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
> > > >  		}
> > > >  
> > > > +# Non-networking without an initial /*
> > > > +		if ($realfile !~ m@^(drivers/net/|net/)@ &&
> > > > +		    $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ &&
> > > > +		    $rawline =~ /^\+[ \t]*\*/) {
> > > > +			WARN("BLOCK_COMMENT_STYLE",
> > > > +			     "multi-line block comments should start with an empty /* line\n" . $hereprev);
> > > > +		}
> > > > +
> > > >  # Block comments use * on subsequent lines
> > > >  		if ($prevline =~ /$;[ \t]*$/ &&			#ends in comment
> > > >  		    $prevrawline =~ /^\+.*?\/\*/ &&		#starting /*    
> > > 
> > > Still nack.  Too many existing instances.
> > > 
> > > $ git grep '/\*.*' -- '*.[ch]' | \
> > >   grep -v '/\*.*\*/' | \
> > >   grep -v -P "/\*\s*$" | \
> > >   grep -v '/\*\*' | \
> > >   grep -v "SPDX-License" | \
> > >   grep -v -P '^drivers/net|^net/' | \
> > >   wc -l
> > > 51834  
> > 
> > Um. Not everything that is currently found in the source tree would be
> > accepted as new code by today's standards...
> > 
> > As it stands, the script checks the special case for net/ and
> > drivers/net/ but does not help prevent unnecessary respins, like this
> > one:
> > 
> > https://lore.kernel.org/linux-iommu/ZRMgObTMkfq8Bjbe@arm.com/
> > 
> > OTOH if we don't want to warn about multi-line comments, maybe we don't
> > want to call it the preferred style, and the corresponding paragraph
> > should be removed from coding-style.rst. Do you suggest I try that
> > instead?  
> 
> If you really want to bring it up as a coding style issue
> go ahead, but consider that the link above is a 'nitpick'
> and not an actual issue.

I don't want to start a flamewar. In fact, I'm quite relaxed about
coding style, so I'm not the right person to propose changes. However,
if enough people believe that most instances found by your grep command
are perfectly valid style, then the text in coding-style.rst does not
match reality...

> If you _really_ want, but I am not at all sure it's useful,
> I suggest you change the message to a CHK so that it is only
> emitted with --strict and not a WARN and only emit anything
> when the thing being scanned is a patch and _not_ a file.

OK, I'm trying to understand what it means when the script WARNs about
something. IIUC these are just warnings about _possible_ coding style
violations; authors and maintainers can choose to ignore them (although
I know some maintainers are more strict than others on this matter).

And then there is existing code. On the one hand, I get a warning when
for BUG_ON(), although there are many existing BUG_ON() instances:

$ git grep -w BUG_ON | wc -l
7561

Yet, there is a check for BUG_ON(). I get a warning even when I merely
modify the argument of a BUG_ON(), e.g. as a result of renaming a struct
member. This makes me believe that warnings from checkpatch.pl are not
meant to be enforced.

On the other hand, you (the maintainer) are not sure if it's useful to
add a warning about block comment style, where the official guidelines
has explicitly asked authors to "use it consistently" since 2006 when
Randy Dunlap wrote commit b3fc9941fbc6e ("CodingStyle updates").

> Something like:
> 
> # Non-networking without an initial /*
> 		if (!$file &&
> 		    $realfile !~ m@^(?:drivers/net/|net/)@ &&
> 		    $prevrawline =~ /^\+[ \t]*\/\*.*[^ \t]$/ &&
> 		    $rawline =~ /^\+[ \t]*\*/) {
> 			CHK("BLOCK_COMMENT_STYLE",
> 			    "multi-line block comments should start with an empty /* line\n" . $hereprev);
> 
> But you still want to examine some of the false positives
> this would create like:
> 
> /* ------------------------
>  * block message
>  * ------------------------ */

OK, I can see this style is often used to mark larger sections.

> and
> 
> struct foo {
> 	int a;  /* some desriptor */
> 	int b;	/* some descriptor
> 		   on multiple lines */

AFAICS this would not trigger a warning in my proposed code, because
the first line of the comment is preceded by non-white-space characters.

I have also seen a lot of instances at the very beginning of a file.
What about skipping the check if a block comment is followed by an empty
line? I was also thinking about minus signs, but I'm not sure if the
following instance is considered perfectly acceptable:

	/* -- Watchdog Timeout --
	 * Bit 0-6 (Reserved)
	 * Bit 7   WDT Time-out Value Units Select
	 *	   (0 = Minutes, 1 = Seconds)
	 */
	outb(timeout_unit, sch311x_wdt_data.runtime_reg + WDT_TIME_OUT);

(found in drivers/watchdog/sch311x_wdt.c)

All I want to achieve is that the check script somehow helps me keep a
consistent style both in network code and non-network code. It's
difficult for me as a human to keep paying attention to all these
little details and subtle differences among maintainers, but at least
some people do care about them. An automated check might help
productivity IMO.

Petr T

Powered by blists - more mailing lists