Messages in this thread | | | Date | Sun, 18 Jul 2021 20:34:59 +0530 (IST) | From | Ani Sinha <> | Subject | Re: [PATCH v3] checkpatch: add a rule to check general block comment style |
| |
On Sun, 18 Jul 2021, Dwaipayan Ray wrote:
> On Sun, Jul 18, 2021 at 7:09 PM Ani Sinha <ani@anisinha.ca> 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. > > > > > > > > > > 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
| |