Messages in this thread | | | From | Lukas Bulwahn <> | Date | Sun, 18 Jul 2021 09:30:11 +0200 | Subject | Re: [PATCH v3] checkpatch: add a rule to check general block comment style |
| |
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. > > > > 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.
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.
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.
I would suggest submitting patches correcting this issue for a significant subsystem, such that this subsystem is clean of violations and then only activate the check in checkpatch for that subsystem. If such patches are accepted and the largest part of the kernel is cleaned up of such violations, we should consider adding such a rule to checkpatch.
Lukas
> > Signed-off-by: Ani Sinha <ani@anisinha.ca> > > --- > > scripts/checkpatch.pl | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > Changelog: > > v1: initial patch > > v2: commit log updated to reflect the output from checkpatch.pl > > when run against the same file both in networking and > > non-networking case. This helps in comparing results apples to > > apples. > > v3: line numbers got lost in the commit log as git eliminated all lines > > starting with '#'. Fixed it by prefixing with word 'line'. The work > > 'line' does not however appear in the checkpatch.pl output. > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > > index 23697a6b1eaa..5f047b762aa1 100755 > > --- a/scripts/checkpatch.pl > > +++ b/scripts/checkpatch.pl > > @@ -3833,6 +3833,14 @@ sub process { > > "networking block comments don't use an empty /* line, use /* Comment...\n" . $hereprev); > > } > > > > +# Block comments use /* on a line of its own > > + if (!($realfile =~ m@^(drivers/net/|net/)@) && > > + $rawline !~ m@^\+.*/\*.*\*/[ \t)}]*$@ && #inline /*...*/ > > + $rawline =~ m@^\+.*/\*\*?+[ \t]*[^ \t]@) { # /* or /** non-blank > > + WARN("BLOCK_COMMENT_STYLE", > > + "Block comments use a leading /* on a separate line\n" . $herecurr); > > + } > > + > > # Block comments use * on subsequent lines > > if ($prevline =~ /$;[ \t]*$/ && #ends in comment > > $prevrawline =~ /^\+.*?\/\*/ && #starting /* > > -- > > 2.25.1 > > > >
| |