Messages in this thread | | | From | Ani Sinha <> | Date | Mon, 19 Jul 2021 12:25:26 +0530 (IST) | Subject | Re: [PATCH v3] checkpatch: add a rule to check general block comment style |
| |
On Mon, 19 Jul 2021, Lukas Bulwahn wrote:
> On Mon, Jul 19, 2021 at 7:28 AM Ani Sinha <ani@anisinha.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@anisinha.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. >
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... > > > > > > > > > >
| |