checkpatch: checker for comment block
cover checkpatch: checker for comment block
Eduardo Habkost pointed out a malformed block of comments on my patch [1] that I had ran checkpatch.pl and no warn/error was reported. Then I realized the script does not catch such as case (or it had a bug).
It turns out that checkpatch.pl does not parse comment blocks (If I understood its code correctly...). So I implemented a checker that warns about:
1. block doesn't begin on its own line.
Example:
/* blah blah
* and blah blah
*/
2. line in the block doesn't start with asterisk. Example:
/*
foo bar
bar foo
*/
Note: only the first occurence (i.e 'foo bar') is reported.
3. block doesn't end on its own line.
Example:
/*
* blah blah
* and blah */
References:
[1] https://www.mail-archive.com/[email protected]/msg580091.html
ps: last time I wrote Perl code was about 13 years ago. That remember me of good times. :)
Wainer dos Santos Moschetta (1):
checkpatch: check for malformed comment block.
scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
Signature · 1 line
2.19.1
PATCH 1/1 checkpatch: check for malformed comment block.
Currently scripts/checkpatch.pl does not check if multiline comments follow the QEMU's coding sytle.
It is added a check for multiline comments that warns about:
1. block doesn't begin on its own line.
2. line in the block doesn't start with asterisk.
3. block doesn't end on its own line.
--- scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
scripts/checkpatch.pl +32 -0
@@ -1858,6 +1858,38 @@ sub process { $line =~ s@//.*@@; $opline =~ s@//.*@@; +# check for malformed comment block. + if ($rawline =~ m{/\*} && $rawline !~ m{\*\/}) { + if ($rawline !~ m{^.\s*/\*+$}) { + WARN("comment block should begin on its own" . + " line\n" . $herecurr); + } + + my $rel_line = 1; + my $reported = 0; + my $comment_line = ''; + my $herecurr = ''; + do { + $comment_line = $rawlines[$rel_line + $linenr + - 1]; + $herecurr = "#".($linenr + $rel_line) . + ": FILE: ".$realfile . + ":".($realline + $rel_line)."\n" . + $comment_line."\n"; + if (!$reported && $comment_line !~ m{^.\s*\*}) { + WARN("line in comment block should" . + " start with asterisk\n" . + $herecurr); + $reported = 1; + } + $rel_line++; + } while ($comment_line && $comment_line !~ m{\*\/}); + + if ($comment_line && $comment_line !~ m{^.\s*\*+\/}) { + WARN("comment block should end on its own" . + " line\n".$herecurr); + } + } # check for global initialisers. if ($line =~ /^.$Type\s*$Ident\s*(?:\s+$Modifier)*\s*=\s*(0|NULL|false)\s*;/) { ERROR("do not initialise globals to 0 or NULL\n" .
Signature · 1 line
2.19.1
Quoted 6 lines · Wainer dos Santos Moschetta
Eduardo Habkost pointed out a malformed block of comments on my patch [1] that I had ran checkpatch.pl and no warn/error was reported. Then I realized the script does not catch such as case (or it had a bug).
Quoted 7 lines
It turns out that checkpatch.pl does not parse comment blocks (If I understood its code correctly...). So I implemented a checker that warns about:
1. block doesn't begin on its own line.
Example:
/* blah blah
* and blah blah
*/
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/
It didn't get applied because Paolo disagreed with having our tools enforcing what our style guide says.
Personally I think we should just commit my patch, and then we can stop having people manually pointing out where submitters' patches don't match CODING_STYLE.
thanks
-- PMM
Quoted 16 lines · Peter Maydell
Quoted 6 lines · Wainer dos Santos Moschetta
Eduardo Habkost pointed out a malformed block of comments on my patch [1] that I had ran checkpatch.pl and no warn/error was reported. Then I realized the script does not catch such as case (or it had a bug).
Quoted 7 lines
It turns out that checkpatch.pl does not parse comment blocks (If I understood its code correctly...). So I implemented a checker that warns about:
1. block doesn't begin on its own line.
Example:
/* blah blah
* and blah blah
*/
Quoted 5 lines
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/It didn't get applied because Paolo disagreed with having our tools enforcing what our style guide says.
I didn't disagree with that---I disagreed with having a single style in the style guide, because unlike most other blatant violations of the coding style (eg. braces), this one is pervasive in maintained code and I don't want code that I maintain to mix two comment styles.
So I proposed two alternatives:
- someone fixes all the comment blocks which are "starred" but don't have a lone "/*" at the beginning, and then we can commit that patch;
- we allow "/* foo" on the first line, except for doc comments and for the first line of the file (author/license block), and fix the style guide accordingly.
Paolo
Quoted 12 lines · Paolo Bonzini
Quoted 5 lines · Peter Maydell
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/It didn't get applied because Paolo disagreed with having our tools enforcing what our style guide says.
I didn't disagree with that---I disagreed with having a single style in the style guide, because unlike most other blatant violations of the coding style (eg. braces), this one is pervasive in maintained code and I don't want code that I maintain to mix two comment styles.
Quoted 8 lines
So I proposed two alternatives:
- someone fixes all the comment blocks which are "starred" but don't have a lone "/*" at the beginning, and then we can commit that patch;
- we allow "/* foo" on the first line, except for doc comments and for the first line of the file (author/license block), and fix the style guide accordingly.
We came to a consensus on the comment style when we discussed the patch which updated CODING_STYLE. I'm not personally
a fan of the result (I used to use "/* foo"), but what we have in the doc is what we achieved consensus for. I'm not going to reopen the "what should block comments look like" style debate.
We have always had older code which isn't following the new style, and our general approach is that we don't do mass-style-fix patches across the whole codebase. If you as a maintainer of a particular sub-area want to do a style update you're free to do that.
thanks
-- PMM
Quoted 22 lines · Peter Maydell
Quoted 12 lines · Paolo Bonzini
Quoted 5 lines · Peter Maydell
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/It didn't get applied because Paolo disagreed with having our tools enforcing what our style guide says.
I didn't disagree with that---I disagreed with having a single style in the style guide, because unlike most other blatant violations of the coding style (eg. braces), this one is pervasive in maintained code and I don't want code that I maintain to mix two comment styles.
Quoted 8 lines
So I proposed two alternatives:
- someone fixes all the comment blocks which are "starred" but don't have a lone "/*" at the beginning, and then we can commit that patch;
- we allow "/* foo" on the first line, except for doc comments and for the first line of the file (author/license block), and fix the style guide accordingly.
Quoted 5 lines
We came to a consensus on the comment style when we discussed the patch which updated
CODING_STYLE. I'm not personally
a fan of the result (I used to use "/* foo"), but what we have in the doc is what we achieved consensus for. I'm not going to reopen the "what should block comments look like" style debate.
Sure, I don't want to do that either. I accept the result of the discussion; I don't accept introducing a new warning that will cause over 700 files to become inconsistent sooner or later. Therefore, the only way to enforce the result of the discussion is to change the existing comments, for example by having a script that maintainers can use to change the existing comments in their files. Having each of us come up with their own script or doing it by hand is probably not a good use of everyone's time.
Alternatively, fixing the style guide can also mean "explain why /* foo is allowed by checkpatch even though it does not match the coding style", without rehashing the discussion.
(BTW it may actually be a good idea to fix _some_ instances of bad coding style, in particular the space-tab sequences and the files where there are maybe 2 or 3 tabs that ended up there by mistake. That's a different topic).
Paolo
Quoted 29 lines · Paolo Bonzini
Quoted 22 lines · Peter Maydell
Quoted 12 lines · Paolo Bonzini
Quoted 5 lines · Peter Maydell
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/It didn't get applied because Paolo disagreed with having our tools enforcing what our style guide says.
I didn't disagree with that---I disagreed with having a single style in the style guide, because unlike most other blatant violations of the coding style (eg. braces), this one is pervasive in maintained code and I don't want code that I maintain to mix two comment styles.
Quoted 8 lines
So I proposed two alternatives:
- someone fixes all the comment blocks which are "starred" but don't have a lone "/*" at the beginning, and then we can commit that patch;
- we allow "/* foo" on the first line, except for doc comments and for the first line of the file (author/license block), and fix the style guide accordingly.
Quoted 5 lines
We came to a consensus on the comment style when we discussed the patch which updated
CODING_STYLE. I'm not personally
a fan of the result (I used to use "/* foo"), but what we have in the doc is what we achieved consensus for. I'm not going to reopen the "what should block comments look like" style debate.
Quoted 3 lines
Sure, I don't want to do that either. I accept the result of the discussion; I don't accept introducing a new warning that will cause over 700 files to become inconsistent sooner or later.
By design, checkpatch.pl only checks *patches*. Existing code doesn't trigger warnings until it gets touched. And then it should arguably be made to conform to CODING_STYLE. So, what's the problem again? :)
Quoted 3 lines
Therefore, the
only way to enforce the result of the discussion is to change the existing comments,
I support cleaning up comment style wholesale[*].
Quoted 4 lines
for example by having a script that maintainers can
use to change the existing comments in their files. Having each of us come up with their own script or doing it by hand is probably not a good use of everyone's time.
Sharing tools is good.
Quoted 8 lines
Alternatively, fixing the style guide can also mean "explain why /* foo is allowed by checkpatch even though it does not match the coding style", without rehashing the discussion.
(BTW it may actually be a good idea to fix _some_ instances of bad coding style, in particular the space-tab sequences and the files where there are maybe 2 or 3 tabs that ended up there by mistake. That's a different topic).
You've since posted patches for that. Thanks.
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Personally I think we should just commit my patch, and then we can stop having people manually pointing out where submitters' patches don't match
CODING_STYLE.
Concur. It has my R-by, modulo a commit message tweak.
[*] Same for other style violations. Yes, it's churn, and yes, it'll mess up git-blame some, but I'm convinced the presence of numerous bad examples costs us more. CODING_STYLE was committed almost a decade ago. If we had cleaned up back then, the churn and the blame would be long forgotten, and we would've spared ourselves plenty of review cycles and quite a few style discussions. It's late, but never too late.
Quoted 31 lines · Markus Armbruster
Quoted 29 lines · Paolo Bonzini
Quoted 27 lines · Peter Maydell
Quoted 12 lines · Paolo Bonzini
Quoted 5 lines · Peter Maydell
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/It didn't get applied because Paolo disagreed with having our tools enforcing what our style guide says.
I didn't disagree with that---I disagreed with having a single style in the style guide, because unlike most other blatant violations of the coding style (eg. braces), this one is pervasive in maintained code and I don't want code that I maintain to mix two comment styles.
Quoted 8 lines
So I proposed two alternatives:
- someone fixes all the comment blocks which are "starred" but don't have a lone "/*" at the beginning, and then we can commit that patch;
- we allow "/* foo" on the first line, except for doc comments and for the first line of the file (author/license block), and fix the style guide accordingly.
We came to a consensus on the comment style when we discussed the patch which updated
CODING_STYLE. I'm not personally
a fan of the result (I used to use "/* foo"), but what we have in the doc is what we achieved consensus for. I'm not going to reopen the "what should block comments look like" style debate.
Quoted 7 lines
Quoted 3 lines
Sure, I don't want to do that either. I accept the result of the discussion; I don't accept introducing a new warning that will cause over 700 files to become inconsistent sooner or later.
By design, checkpatch.pl only checks *patches*. Existing code doesn't trigger warnings until it gets touched. And then it should arguably be made to conform to
CODING_STYLE. So, what's the problem again? :)
Once you add multiline comments to a file that has 3-line comments, they have to be 4-line in order to appease checkpatch, and this create inconsistencies.
In other words, it's not about checkpatch complaining on existing code.
However, by running checkpatch on existing maintained code, you get a
measure of which files will grow an inconsistent style unless cleaned up wholesale.
Anyway, in order to conclude the discussion and walk the walk, here is the script. It also converts GNU style to the anointed one. I now support applying Peter's patch, after the script is reviewed I'll send it as a formal patch.
#! /bin/sh
#
# Fix multiline comments to match CODING_STYLE
#
# Copyright (C) 2018 Red Hat, Inc.
#
# Author: Paolo Bonzini
#
# Usage: scripts/fix-multiline-comments.sh [-i] FILE...
#
# -i edits the file in place (requires gawk 4.1.0).
#
# Set the AWK environment variable to choose the awk interpreter to use # (default 'awk')
if test "$1" = -i; then
# gawk extension
inplace="-i inplace"
shift
fi
${AWK-awk} $inplace 'BEGIN {
getline first
indent = -1
while ((getline second) > 0) {
# apply a star to the indent on lines after the first
if (indent != -1) {
if (first == "") {
first = sp " *"
} else if (substr(first, 1, indent + 2) == sp " ") {
first = sp " *" substr(first, indent + 3)
}
} is_lead = (first ~ /^[ \t]*\/\*/)
is_trail = (first ~ /\*\//)
if (is_lead && !is_trail) {
# grab the indent at the start of a comment, but not for
# single-line comments
match(first, /^[ \t]*\/\*/)
indent = RLENGTH - 2
sp = substr(first, 1, indent)
}
# the regular expression filters out lone /*, /**, or */
if (indent != -1 && !(first ~ /^[ \t]*(\/\*+|\*\/)[ \t]*$/)) {
if (is_trail) {
# split the trailing */ on a separate line
match(first, /[ \t]*\*\//)
first = substr(first, 1, RSTART - 1) "\n" sp " */"
}
if (is_lead) {
# split the leading /* on a separate line
match(first, /^[ \t]*\/\*+[ \t]*/)
first = sp "/*\n" sp " *" substr(first, RLENGTH)
}
}
if (is_trail) {
indent = -1
}
print first
first = second
}
print first
}' "$@"Paolo
Quoted 6 lines · Markus Armbruster
Quoted 3 lines · Paolo Bonzini
Quoted 2 lines · Peter Maydell
Quoted 1 line · Paolo Bonzini
On 13/12/18 19:01, Peter Maydell wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/Personally I think we should just commit my patch, and then we can stop having people manually pointing out where submitters' patches don't match
CODING_STYLE.Concur. It has my R-by, modulo a commit message tweak.
I have to admit I never really understood what tweak you wanted making to the commit message. I'm happy to make it clearer: do you want to suggest a rewording?
thanks
-- PMM
Quoted 14 lines · Peter Maydell
Quoted 13 lines · Markus Armbruster
Quoted 9 lines · Paolo Bonzini
Quoted 8 lines · Peter Maydell
Quoted 7 lines · Paolo Bonzini
Quoted 6 lines · Peter Maydell
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/Personally I think we should just commit my patch, and then we can stop having people manually pointing out where submitters' patches don't match
CODING_STYLE.
Quoted 5 lines
Quoted 1 line
Concur. It has my R-by, modulo a commit message tweak.
I have to admit I never really understood what tweak you wanted making to the commit message. I'm happy to make it clearer: do you want to suggest a rewording?
The commit message claims "(The only changes needed are that Linux's checkpatch.pl WARN() function takes an extra argument that ours does not.)" This isn't the case. You also dropped the kernel's "Networking with an initial /*" special case.
The simplest way to fix an incorrect claim is to delete it :)
If you want to explain what you changed, you could say something like "(The only changes needed are that Linux's checkpatch.pl WARN() function takes an extra argument that ours does not, and the kernel has a special case for networking code we don't want.)"
Quoted 10 lines · Markus Armbruster
Quoted 4 lines · Peter Maydell
On Fri, 14 Dec 2018 at 06:29, Markus Armbruster <[email protected]> wrote:
I have to admit I never really understood what tweak
you wanted making to the commit message. I'm happy
to make it clearer: do you want to suggest a rewording?The commit message claims "(The only changes needed are that Linux's checkpatch.pl
WARN()function takes an extra argument that ours does not.)" This isn't the case. You also dropped the kernel's "Networking with an initial /*" special case.
The bit of the commit message you didn't quote says "by backporting the relevant
parts of the Linux kernel's checkpatch.pl. (The only changes needed are that Linux's checkpatch.pl WARN() function takes an extra argument that ours does not.)"
The networking special case is not a "relevant part", which is why it's not in the patch. The bracketed statement applies to the code in the patch, not any lumps of code in the kernel's checkpatch that are not in the patch. Anyway, I've adjusted the commit message as you suggest.
Since we seem to now have consensus on the checkpatch patch, I'm going to put it into the "misc" pull request I'm putting together.
thanks
-- PMM
Quoted 18 lines · Peter Maydell
Quoted 10 lines · Markus Armbruster
Quoted 4 lines · Peter Maydell
On Fri, 14 Dec 2018 at 06:29, Markus Armbruster <[email protected]> wrote:
I have to admit I never really understood what tweak
you wanted making to the commit message. I'm happy
to make it clearer: do you want to suggest a rewording?The commit message claims "(The only changes needed are that Linux's checkpatch.pl
WARN()function takes an extra argument that ours does not.)" This isn't the case. You also dropped the kernel's "Networking with an initial /*" special case.The bit of the commit message you didn't quote says "by backporting the relevant
parts of the Linux kernel's checkpatch.pl. (The only changes needed are that Linux's checkpatch.plWARN()function takes an extra argument that ours does not.)"
Quoted 4 lines
The networking special case is not a "relevant part", which is why it's not in the patch. The bracketed statement applies to the code in the patch, not any lumps of code in the kernel's checkpatch that are not in the patch.
I understand you logic now.
Quoted 5 lines
Anyway, I've adjusted the commit message as you suggest.
Since we seem to now have consensus on the checkpatch patch, I'm going to put it into the "misc" pull request I'm putting together.
Thanks!
Quoted 14 lines · Peter Maydell
Quoted 5 lines · Wainer dos Santos Moschetta
Eduardo Habkost pointed out a malformed block of comments on my patch [1] that I had ran checkpatch.pl and no warn/error was reported. Then I realized the script does not catch such as case (or it had a bug).
Quoted 7 lines
It turns out that checkpatch.pl does not parse comment blocks (If I understood its code correctly...). So I implemented a checker that warns about:
1. block doesn't begin on its own line.
Example:/* blah blah * and blah blah */
Quoted 2 lines
I sent a patch to do this a little while back:
https://patchwork.kernel.org/patch/10561557/
Self-NACK my patch in favor of that, which has additional checks (e.g. * alignment).
Quoted 7 lines
It didn't get applied because Paolo disagreed with having our tools enforcing what our style guide says.
Personally I think we should just commit my patch, and then we can stop having people manually pointing out where submitters' patches don't match
CODING_STYLE.
I am afraid that I can't give a firm option on this topic because it precedes my existence in this community, regardless I tend to agreed on Peter's reasoning.
Thanks!
- Wainer
T
Quoted 3 lines
thanks
-- PMM