lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Mon, 19 Jul 2021 10:58:30 +0530 (IST) From: Ani Sinha <ani@...sinha.ca> To: Joe Perches <joe@...ches.com> cc: Ani Sinha <ani@...sinha.ca>, 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>, Dwaipayan Ray <dwaipayanray1@...il.com> Subject: Re: [PATCH v3] checkpatch: add a rule to check general block comment style 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 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. > 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