Message-ID: <CABJPP5Cyv=0EgMy-Y3AHHHAY0n1OUZ_cDH0Svd=6SkFYOwmPjA@mail.gmail.com>
Date:   Sun, 18 Jul 2021 19:52:34 +0530
From:   Dwaipayan Ray <dwaipayanray1@...il.com>
To:     Ani Sinha <ani@...sinha.ca>
Cc:     Lukas Bulwahn <lukas.bulwahn@...il.com>,
        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>
Subject: Re: [PATCH v3] checkpatch: add a rule to check general block comment style

On Sun, Jul 18, 2021 at 7:09 PM Ani Sinha <ani@...sinha.ca> wrote:
>
>
>
> On Sun, 18 Jul 2021, Lukas Bulwahn wrote:
>
> > 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.
>
> OK. However, I fail to see how your above comment is useful without any
> suggestion as to how to improve the commit log. I find having some test
> data with the commit message valuable so that there is some sort of record
> as to how the change was tested and with what arguments. Beyond that this
> is not something I am really worried about. The commit message can be
> modified and improved in any way reviewers like.
>

A simple tested with:
$cat test.c
/* This is a
 * multi-line comment
 */

would have worked IMO. The commit log proposed is highly
incomprehensible as to what the patch adds. I don't find
it readable either.

> >
> > 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.
>
> I have some strong opinions on this. Just because a rule has been violated
> in the past does not mean it can continue to be violated in the future.
> When violating patches were pushed, perhaps the commenting rule was not in
> place? Perhaps, due to the absense of the checkpatch rule, the author of
> the patch did not pay attention to the comment rule which, btw, exists in
> written form in the kernel doc? Perhaps the people who reviewed the patch
> overlooked it because of the very same reason - checkpatch allowed it?
> If we put the rule in checkpatch, what it means is that all future commits
> will not ignore the commneting rule because checkpatch will draw
> attention to it. Further, this means that there will be potentially no new
> violations. While that is being ensured, we can incrementally fix the
> existing code elsewhere in the tree so that eventually we can converge (no
> violations of this rule anywhere in the kernel source tree).
>

Comment style is one of the top violations we have in the kernel today.
It's a rather trivial thing. See the top checkpatch rule violations:

1797862 CHECK:LONG_LINE:
667040 CHECK:CAMELCASE:
247672 ERROR:SPACING:
168415 CHECK:PARENTHESIS_ALIGNMENT:
124413 CHECK:SPACING:
108615 WARNING:LEADING_SPACE:
64225 CHECK:LINE_SPACING:
54424 CHECK:PREFER_KERNEL_TYPES:
45502 CHECK:BIT_MACRO:
43045 WARNING:BLOCK_COMMENT_STYLE:

I highly disagree that it's because checkpatch allowed it.
We wouldn't be seeing such a high quantity of
other violations otherwise which checkpatch doesn't like.

Coming to what adding this change would mean:
$ git grep -P "^\s*/\*\s*[^/\*]+$" | wc -l
80635

This shall triple the number of WARNING:BLOCK_COMMENT_STYLE
violations. Not sure that's a good thing. And even more unsure
that's a thing anyone should even attempt fixing with other
important things existing.

Dwaipayan.

Powered by blists - more mailing lists