Message-ID: <CAKXUXMyJ9Ab_Fvpk8WAimEnDjcOhJRe2uODkC=RxCgsN8T0WyQ@mail.gmail.com>
Date:   Mon, 19 Jul 2021 07:58:55 +0200
From:   Lukas Bulwahn <lukas.bulwahn@...il.com>
To:     Ani Sinha <ani@...sinha.ca>
Cc:     Joe Perches <joe@...ches.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        anirban.sinha@...ia.com, mikelley@...rosoft.com,
        Andy Whitcroft <apw@...onical.com>,
        Dwaipayan Ray <dwaipayanray1@...il.com>
Subject: Re: [PATCH v3] checkpatch: add a rule to check general block comment style

On Mon, Jul 19, 2021 at 7:28 AM Ani Sinha <ani@...sinha.ca> wrote:
>
>
>
> On Sun, 18 Jul 2021, Joe Perches wrote:
>
> > On Sun, 2021-07-18 at 19:08 +0530, Ani Sinha 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.
> > []
> > > > 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.
> > >
> > > >
> > > > 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.
> >
> > Intensity of opinion varies considerably here.
> >
> > > > 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.
> >
> > The only reason the rule exists at all is because the networking maintainer
> > was constantly telling people to change the comment style in patches.
> >
> > I don't care one way or another.
> >
> > // comments are fine
> > /* comments are fine */
> >
> > In networking, multiline comments are almost exclusively like
> > what Linus himself does not like:
> >
> >       /* comment
> >        * ...
> >        */
> >
> > but in other subsystems, the styles of multiline comments varies.
> >
> > Either works, there is no single standard.
> >
>
> OK then in that case, maybe update
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v5.14-rc2#n584
>

The rule may still hold.

> It is confusing to patch submitters (and it happened to me with a recent
> patch) that the reviewer insists on a particular commenting style when
> checkpatch does not enforce it. Its confusing for reviewers too.
>

I think this is more about the confusion of what you should really
expect from checkpatch.

Checkpatch does some checking, but this checking is not sound, i.e.,
perfect wrt. expectations on submissions, i.e., there are various
cases where checkpatch's suggestion is NOT the
community's/maintainer's preference. Some rules are even quite basic
heuristics, and can get confused by unexpected patterns.

Hence, in its current state, with all rules enabled, you could not
enforce a checkpatch pre-commit hook as you suggested.

Further, checkpatch is not complete either: just because checkpatch
did not complain on some stylistic issues does not mean that all rules
on style that might be automatically checkable are followed in a
patch. During the patch submission, reviewers might still ask for
further stylistic improvements, even if checkpatch did not point them
out.

Contributing to checkpatch improvements is certainly welcome. However,
it is a non-trivial task to include checks that make checkpatch more
usable (more accepted among developers and maintainers) with the
current submission practice and the currently existing code in the
kernel repository.

Lukas

>
> > And as the referenced link by Aditya somewhat shows, the nominal
> > rule compliance varies by the age of the code.  No one care much
> > about code submitted a couple decades ago for subsystems and drivers
> > that are effectively obsolete...
> >
> >
> >

Powered by blists - more mailing lists