Re: [PATCH v3] checkpatch: add a rule to check general block comment style

[Ani Sinha]

On Mon, 19 Jul 2021, Lukas Bulwahn wrote:

> On Mon, Jul 19, 2021 at 7:28 AM Ani Sinha <ani@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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@xxxxxxxxx/.
> > > > > 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.
>

Hence, I do not see why we cannot add this rule to checkpatch. If the
reviewer likes the other style of commenting they can always ask for a
correction. Having checkpatch agree with Linus' preferred style of
commenting and the preferred documeted style of commenting (which seems to
be the same) does make everything uniform and agreeable.

> > 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...
> > >
> > >
> > >
>