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.
Signed-off-by: Ani Sinha <[email protected]>
---
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
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.
>
> Signed-off-by: Ani Sinha <[email protected]>
> ---
> 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
>
>
On Fri, Jul 16, 2021 at 6:15 PM Ani Sinha <[email protected]> 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/[email protected]/.
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 <[email protected]>
> > ---
> > 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
> >
> >
On Sun, 18 Jul 2021, Lukas Bulwahn wrote:
> On Fri, Jul 16, 2021 at 6:15 PM Ani Sinha <[email protected]> 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.
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.
When violating patches were pushed, perhaps the commenting rule was not in
place? Perhaps, due to the absense of the checkpatch rule, the author of
the patch did not pay attention to the comment rule which, btw, exists in
written form in the kernel doc? Perhaps the people who reviewed the patch
overlooked it because of the very same reason - checkpatch allowed it?
If we put the rule in checkpatch, what it means is that all future commits
will not ignore the commneting rule because checkpatch will draw
attention to it. Further, this means that there will be potentially no new
violations. While that is being ensured, we can incrementally fix the
existing code elsewhere in the tree so that eventually we can converge (no
violations of this rule anywhere in the kernel source tree).
>
> 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/[email protected]/.
> So, this checkpatch check is largely wrong already as of now and most
> probably ignored by many contributors.
>
If this rule is being ignored, then another option is to simply remove the
rule. A rule without eny enforcement and which is easy to ignore is bogus,
IMHO.
> I would suggest submitting patches correcting this issue for a
> significant subsystem, such that this subsystem is clean of violations
Seems a subsystem clean of all violations is a non starter IMHO. We should
not expect any such subsystems in the tree. Rather, a better approach is
to put the rule in place today and then incrementally clean up the
subsystems of existing 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.
Without a tool enforcing a rule, we will never be able to converge. I am
speaking from experience of writing post commit hooks for a company which
enforced pylint violations.
Ani
>
>
> Lukas
>
> > > Signed-off-by: Ani Sinha <[email protected]>
> > > ---
> > > 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
> > >
> > >
>
On Sun, Jul 18, 2021 at 7:09 PM Ani Sinha <[email protected]> wrote:
>
>
>
> On Sun, 18 Jul 2021, Lukas Bulwahn wrote:
>
> > On Fri, Jul 16, 2021 at 6:15 PM Ani Sinha <[email protected]> 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.
>
> 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.
>
A simple tested with:
$cat test.c
/* This is a
* multi-line comment
*/
would have worked IMO. The commit log proposed is highly
incomprehensible as to what the patch adds. I don't find
it readable either.
> >
> > 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.
> When violating patches were pushed, perhaps the commenting rule was not in
> place? Perhaps, due to the absense of the checkpatch rule, the author of
> the patch did not pay attention to the comment rule which, btw, exists in
> written form in the kernel doc? Perhaps the people who reviewed the patch
> overlooked it because of the very same reason - checkpatch allowed it?
> If we put the rule in checkpatch, what it means is that all future commits
> will not ignore the commneting rule because checkpatch will draw
> attention to it. Further, this means that there will be potentially no new
> violations. While that is being ensured, we can incrementally fix the
> existing code elsewhere in the tree so that eventually we can converge (no
> violations of this rule anywhere in the kernel source tree).
>
Comment style is one of the top violations we have in the kernel today.
It's a rather trivial thing. See the top checkpatch rule violations:
1797862 CHECK:LONG_LINE:
667040 CHECK:CAMELCASE:
247672 ERROR:SPACING:
168415 CHECK:PARENTHESIS_ALIGNMENT:
124413 CHECK:SPACING:
108615 WARNING:LEADING_SPACE:
64225 CHECK:LINE_SPACING:
54424 CHECK:PREFER_KERNEL_TYPES:
45502 CHECK:BIT_MACRO:
43045 WARNING:BLOCK_COMMENT_STYLE:
I highly disagree that it's because checkpatch allowed it.
We wouldn't be seeing such a high quantity of
other violations otherwise which checkpatch doesn't like.
Coming to what adding this change would mean:
$ git grep -P "^\s*/\*\s*[^/\*]+$" | wc -l
80635
This shall triple the number of WARNING:BLOCK_COMMENT_STYLE
violations. Not sure that's a good thing. And even more unsure
that's a thing anyone should even attempt fixing with other
important things existing.
Dwaipayan.
On Sun, 18 Jul 2021, Dwaipayan Ray wrote:
> On Sun, Jul 18, 2021 at 7:09 PM Ani Sinha <[email protected]> wrote:
> >
> >
> >
> > On Sun, 18 Jul 2021, Lukas Bulwahn wrote:
> >
> > > On Fri, Jul 16, 2021 at 6:15 PM Ani Sinha <[email protected]> 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.
> >
> > 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.
> >
>
> A simple tested with:
> $cat test.c
> /* This is a
> * multi-line comment
> */
>
> would have worked IMO. The commit log proposed is highly
> incomprehensible as to what the patch adds. I don't find
> it readable either.
OK if there is any interest in this patch (which atm does not seem to be
any), I will shorten the commit log as per the above suggestion.
>
> > >
> > > 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.
> > When violating patches were pushed, perhaps the commenting rule was not in
> > place? Perhaps, due to the absense of the checkpatch rule, the author of
> > the patch did not pay attention to the comment rule which, btw, exists in
> > written form in the kernel doc? Perhaps the people who reviewed the patch
> > overlooked it because of the very same reason - checkpatch allowed it?
> > If we put the rule in checkpatch, what it means is that all future commits
> > will not ignore the commneting rule because checkpatch will draw
> > attention to it. Further, this means that there will be potentially no new
> > violations. While that is being ensured, we can incrementally fix the
> > existing code elsewhere in the tree so that eventually we can converge (no
> > violations of this rule anywhere in the kernel source tree).
> >
>
> Comment style is one of the top violations we have in the kernel today.
> It's a rather trivial thing. See the top checkpatch rule violations:
>
> 1797862 CHECK:LONG_LINE:
> 667040 CHECK:CAMELCASE:
> 247672 ERROR:SPACING:
> 168415 CHECK:PARENTHESIS_ALIGNMENT:
> 124413 CHECK:SPACING:
> 108615 WARNING:LEADING_SPACE:
> 64225 CHECK:LINE_SPACING:
> 54424 CHECK:PREFER_KERNEL_TYPES:
> 45502 CHECK:BIT_MACRO:
> 43045 WARNING:BLOCK_COMMENT_STYLE:
>
> I highly disagree that it's because checkpatch allowed it.
> We wouldn't be seeing such a high quantity of
> other violations otherwise which checkpatch doesn't like.
>
Perhaps the subsystems maintainers should mandatorily make sure the
patches they apply pass checkpatch? It is something like a pre-commit
hook where the commit fails if the pre-commit checks do not pass.
> Coming to what adding this change would mean:
> $ git grep -P "^\s*/\*\s*[^/\*]+$" | wc -l
> 80635
>
> This shall triple
43045 -> 80635 is barely double, not triple.
the number of WARNING:BLOCK_COMMENT_STYLE
> violations. Not sure that's a good thing. And even more unsure
> that's a thing anyone should even attempt fixing with other
> important things existing.
A rule without enforcement mechanism through tooling is useless. I rest my
case.
Ani
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 <[email protected]> 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/[email protected]/.
> > 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.
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...
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 <[email protected]> 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/[email protected]/.
> > > 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...
>
>
>
On Mon, Jul 19, 2021 at 7:28 AM Ani Sinha <[email protected]> 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 <[email protected]> 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/[email protected]/.
> > > > 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...
> >
> >
> >
On Mon, 19 Jul 2021, Lukas Bulwahn wrote:
> On Mon, Jul 19, 2021 at 7:28 AM Ani Sinha <[email protected]> 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 <[email protected]> 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/[email protected]/.
> > > > > 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...
> > >
> > >
> > >
>
On Mon, 2021-07-19 at 12:25 +0530, Ani Sinha wrote:
> 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.
Too many novice developers take checkpatch output as dicta.
It's not.
It's just produces suggestions that should _always_ be taken
not very seriously. Those suggestions should perhaps be
considered, but good taste should always override a brainless
script.
_Very_ few senior developers really care that much about any
particular comment style.
These are the same senior developers that would be burdened
with unnecessary patches to review from those novice developers
that believe checkpatch should always be followed.
Do not unnecessarily burden senior developers.
They are generally have other priorities.
On Mon, 19 Jul 2021, Joe Perches wrote:
> On Mon, 2021-07-19 at 12:25 +0530, Ani Sinha wrote:
>
> > 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.
>
> Too many novice developers take checkpatch output as dicta.
>
> It's not.
>
Well those "novice" developers have perhaps worked in companies where
tooling like pre-commit sanity hooks have provided immense value in
ensuring certain basic rules and code quality is maintained across the
board, particulay when the company scales. Existing violations did not
deter them from adding stricter rules to make sure all future commits
follow certain patterns. Ofcourse at the end of the day, common sense
trumps any tooling, goes without saying.
> It's just produces suggestions that should _always_ be taken
> not very seriously. Those suggestions should perhaps be
> considered, but good taste should always override a brainless
> script.
At the very very least, checkpatch should lay this out in clear terms
every time this is run. Different communities have different rules and for
me, I always run all my patches through checkpatch to make sure that the
patch I sent out of review at least is checkpatch clean. This makes sure
that I am not violating any obvious code submission rules laid out by that
community. This is particularly true for kernel community where flaming
people for even small issues seems to be the culture!
>
> _Very_ few senior developers really care that much about any
> particular comment style.
I disagree on this.
>
> These are the same senior developers that would be burdened
> with unnecessary patches to review from those novice developers
> that believe checkpatch should always be followed.
>
Well for those "novice" developers, the doc tells us to run checkpatch
and address the complaints :
Are you sure your patch is free of silly mistakes? You should always
run patches through scripts/checkpatch.pl and address the complaints it
comes up with.
Anyways it seems this conversation is self serving for the kernel's sr
developers so that they can take any stance convenient to them.
There is no value.
On Mon, Jul 19, 2021 at 10:08 AM Ani Sinha <[email protected]> wrote:
>
>
>
> On Mon, 19 Jul 2021, Joe Perches wrote:
>
> > On Mon, 2021-07-19 at 12:25 +0530, Ani Sinha wrote:
> >
> > > 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.
> >
> > Too many novice developers take checkpatch output as dicta.
> >
> > It's not.
> >
>
> Well those "novice" developers have perhaps worked in companies where
> tooling like pre-commit sanity hooks have provided immense value in
> ensuring certain basic rules and code quality is maintained across the
> board, particulay when the company scales. Existing violations did not
> deter them from adding stricter rules to make sure all future commits
> follow certain patterns. Ofcourse at the end of the day, common sense
> trumps any tooling, goes without saying.
>
> > It's just produces suggestions that should _always_ be taken
> > not very seriously. Those suggestions should perhaps be
> > considered, but good taste should always override a brainless
> > script.
>
> At the very very least, checkpatch should lay this out in clear terms
> every time this is run. Different communities have different rules and for
> me, I always run all my patches through checkpatch to make sure that the
> patch I sent out of review at least is checkpatch clean. This makes sure
> that I am not violating any obvious code submission rules laid out by that
> community. This is particularly true for kernel community where flaming
> people for even small issues seems to be the culture!
>
You are assuming that there is THE one consistent style in the kernel
repository and within the whole kernel community. But, IMHO, there is
not; the repository is too large and the community is too diverse in
its preferences. Do an evaluation of coding style in the whole
repository and share which one rule is consistently followed in all
directories (and how many are not).
Further, checking as pre-commit hooks requires that a repository is
rather clean wrt. to rule violations, but often in the kernel, it is
not. So, you then need to clean up the existing code, which causes
dangerous large syntactic refactorings and drowns maintainers. That
has been tried, but has been not successful in the past.
Hence, with the current state, the checkpatch tool needs to be handled
much more with care and critical consideration of the state of the
actual code base and the rationales of the rules that checkpatch
points out.
Surely, we need better documentation to point out how to use
checkpatch, what the reasons for certain rules are, and why some
simply need to be ignored on certain files. Evaluations and
contributions are welcome.
> >
> > _Very_ few senior developers really care that much about any
> > particular comment style.
>
> I disagree on this.
>
> >
> > These are the same senior developers that would be burdened
> > with unnecessary patches to review from those novice developers
> > that believe checkpatch should always be followed.
> >
>
> Well for those "novice" developers, the doc tells us to run checkpatch
> and address the complaints :
>
> Are you sure your patch is free of silly mistakes? You should always
> run patches through scripts/checkpatch.pl and address the complaints it
> comes up with.
>
>
> Anyways it seems this conversation is self serving for the kernel's sr
> developers so that they can take any stance convenient to them.
> There is no value.
>
> Surely, we need better documentation to point out how to use
> checkpatch, what the reasons for certain rules are, and why some
> simply need to be ignored on certain files. Evaluations and
> contributions are welcome.
>
This has to come from the experienced sr. kernel subsystem maintainers
and checkpatch maintainers so that they can educate us, the novice
contributors, in the right way to use checkpatch and know the rules
associated with it.