Message-ID: <CAKXUXMzxyQxYKdnuVbZnUFbYFuKMq+Vc5LxSR-_fKtH2dv-4wA@mail.gmail.com>
Date:   Sun, 18 Jul 2021 09:30:11 +0200
From:   Lukas Bulwahn <lukas.bulwahn@...il.com>
To:     Ani Sinha <ani@...sinha.ca>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        anirban.sinha@...ia.com, mikelley@...rosoft.com,
        Andy Whitcroft <apw@...onical.com>,
        Joe Perches <joe@...ches.com>,
        Dwaipayan Ray <dwaipayanray1@...il.com>
Subject: Re: [PATCH v3] checkpatch: add a rule to check general block comment style

On Fri, Jul 16, 2021 at 6:15 PM Ani Sinha <ani@...sinha.ca> wrote:
>
> checkpatch maintainers, any comments?
>
> On Wed, 14 Jul 2021, Ani Sinha wrote:
>
> > 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.
> >        */
> >
> > It seems rule in checkpatch.pl is missing to ensure this for
> > non-networking related changes. This patch adds this rule.
> >
> > Tested with
> > $ cat drivers/net/t.c
> >     /* foo */
> >
> >     /*
> >      * foo
> >      */
> >
> >     /* foo
> >      */
> >
> >     /* foo
> >      * bar */
> >
> > $ ./scripts/checkpatch.pl -f drivers/net/t.c
> > WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> > line #1: FILE: drivers/net/t.c:1:
> > +    /* foo */
> >
> > WARNING: networking block comments don't use an empty /* line, use /* Comment...
> > line #4: FILE: drivers/net/t.c:4:
> > +    /*
> > +     * foo
> >
> > WARNING: Block comments use a trailing */ on a separate line
> > line #11: FILE: drivers/net/t.c:11:
> > +     * bar */
> >
> > total: 0 errors, 3 warnings, 0 checks, 11 lines checked
> >
> >
> > For a non-networking related code we see the following when run for
> > the same file:
> >
> > $ ./scripts/checkpatch.pl -f arch/x86/kernel/t.c
> > WARNING: Missing or malformed SPDX-License-Identifier tag in line 1
> > line #1: FILE: arch/x86/kernel/t.c:1:
> > +    /* foo */
> >
> > WARNING: Block comments use a leading /* on a separate line
> > line #7: FILE: arch/x86/kernel/t.c:7:
> > +    /* foo
> >
> > WARNING: Block comments use a leading /* on a separate line
> > line #10: FILE: arch/x86/kernel/t.c:10:
> > +    /* foo
> >
> > WARNING: Block comments use a trailing */ on a separate line
> > line #11: FILE: arch/x86/kernel/t.c:11:
> > +     * bar */
> >
> > total: 0 errors, 4 warnings, 11 lines checked
> >
> > In the second case, there is no warning on line 4 and in the first
> > case, there is no warning on line 10.
> >

Honest feedback: IMHO, your commit message is unreadable and incomprehensible.

Now to the feature you are proposing:

I do not think that it is good if checkpatch would point out a quite
trivial syntactic issue that probably is currently violated many times
(>10,000 or even >100,000 times?) in the overall repository. That will
make checkpatch warn on many commits with this check and divert the
attention from other checks that are more important than the style of
starting comments.

Further, some evaluation by Aditya shows that the distinction between
NETWORKING COMMENT STYLE and GENERAL KERNEL COMMENT STYLE is not as
easily split as currently encoded in the checkpatch script,
https://lore.kernel.org/linux-kernel-mentees/cfff5784-9ca3-07f8-c51c-f1c82b2871e3@gmail.com/.
So, this checkpatch check is largely wrong already as of now and most
probably ignored by many contributors.

I would suggest submitting patches correcting this issue for a
significant subsystem, such that this subsystem is clean of violations
and then only activate the check in checkpatch for that subsystem.
If such patches are accepted and the largest part of the kernel is
cleaned up of such violations, we should consider adding such a rule
to checkpatch.


Lukas

> > Signed-off-by: Ani Sinha <ani@...sinha.ca>
> > ---
> >  scripts/checkpatch.pl | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > Changelog:
> > v1: initial patch
> > v2: commit log updated to reflect the output from checkpatch.pl
> >     when run against the same file both in networking and
> >     non-networking case. This helps in comparing results apples to
> >     apples.
> > v3: line numbers got lost in the commit log as git eliminated all lines
> >     starting with '#'. Fixed it by prefixing with word 'line'. The work
> >     'line' does not however appear in the checkpatch.pl output.
> >
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 23697a6b1eaa..5f047b762aa1 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3833,6 +3833,14 @@ sub process {
> >                            "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev);
> >               }
> >
> > +# Block comments use /* on a line of its own
> > +             if (!($realfile =~ m@^(drivers/net/|net/)@) &&
> > +                 $rawline !~ m@^\+.*/\*.*\*/[ \t)}]*$@ &&    #inline /*...*/
> > +                 $rawline =~ m@^\+.*/\*\*?+[ \t]*[^ \t]@) { # /* or /** non-blank
> > +                 WARN("BLOCK_COMMENT_STYLE",
> > +                      "Block comments use a leading /* on a separate line\n" . $herecurr);
> > +             }
> > +
> >  # Block comments use * on subsequent lines
> >               if ($prevline =~ /$;[ \t]*$/ &&                 #ends in comment
> >                   $prevrawline =~ /^\+.*?\/\*/ &&             #starting /*
> > --
> > 2.25.1
> >
> >

Powered by blists - more mailing lists