Re: [Qemu-devel] [PATCH] scripts/checkpatch.pl: Enforce multiline comment syntax

[Paolo Bonzini <pbonzini@redhat.com>]

On 10/08/2018 14:53, Peter Maydell wrote:
>> But otherwise, at least Eric, you, me (only now I admit), Thomas
>> expressed a preference for the other style; on the other side it's
>> Markus, Stefan, Conny and Alex, some of whom were okay with applying
>> maintainer discretion; John and rth wanted a third one but disagreed on
>> their second choice.  I appreciate your writing the patch, but I'm not
>> sure that's consensus...
> What I strongly want is that checkpatch should (a) catch stuff
> I get wrong and (b) catch stuff that other people get wrong,
> so I don't have to nitpick over coding style myself (which I
> have done for multiline comment style in the past). So to me
> the current situation (checkpatch doesn't warn at all about
> out-of-style multiline comments) is no good.
> 
> Nobody runs checkpatch on the whole existing codebase anyway,
> do they? So I think "3000 new warnings" is a red herring.

It's just a proxy for how many file would become inconsistent in the long
run, by respecting the new rule for now code and not changing the existing
code.  So let's try doing it: :)

  git ls-tree -r HEAD | grep -v 'tests/tcg' | grep -v disas | \
    grep -v capstone | grep -v libxl | grep -v libdecnumber | \
    grep '\.[ch]$' | while read f g h i; do \
      scripts/checkpatch.pl -f $i;
  done | tee checkpatch.list

First of all, there were no hangs and only one file took a long time (ui/vgafont.h)
because it hits a quadratic loop; that was a nice start.

We have:

	Tested files: 3392
	Zero errors and zero warnings: 1938
	Zero errors, some warnings: 152
	Total errors + warnings: 82700 (only ~4000 warnings)

	Top reports:
	44425 ERROR: code indent should never use tabs
	19072 ERROR: space required / space prohibited
	4893 ERROR: braces {} are necessary for all arms
	3573 WARNING: line over 80 characters
	1198 ERROR: do not use C99 // comments
	1090 ERROR: line over 90 characters
	1053 ERROR: trailing statements should be on next line

So about 65% of the files pass before the patch, which is much better 
than I actually thought.

A note about tabs: only 360 lines have a space-tab sequence, and
perhaps we could fix those.  18950 lines have tabs only at the beginning
of the line.  23405 lines have a single sequence of tabs in the middle
of the line but the indentation is otherwise absent or space-based.
But I digress.

With Peter's patch:

	Zero errors and zero warnings: 1269
	Zero errors, some warnings: 895

	New warnings:
	13214 WARNING: Block comments use a leading /* on a separate line
	4392 WARNING: Block comments use a trailing */ on a separate line
	2762 WARNING: Block comments use * on subsequent lines
	117 WARNING: Block comments should align the * on each line

With alternate rule (and dispensation for line 1):

	Zero errors and warnings: 1435
	Zero errors, some warnings: 831

	New warnings:
	4891 WARNING: Block comments use a leading /* on a separate line
	4392 WARNING: Block comments use a trailing */ on a separate line
	2762 WARNING: Block comments use * on subsequent lines
	117 WARNING: Block comments should align the * on each line

Given the number of warnings about trailing */, the 2-line format is much more
prevalent than I thought.  We can assume that there are no comments with
separate "/*" and joined "*/", and then by comparing the different number
of warnings in the two cases you get the following estimates:

- 13713 multiline comments (13214+4891-4392)
- 1630 multiline comments in "2-line" format with asterisks (4392-2762)
- 2762 multiline comments in "2-line" format without asterisks
- 4430 multiline comments in "3-line" format
- 2024 doc comments (should be in "4-line" format, I spot checked a few dozen;
  this was from a separate grep for "/\*\*$")
- 2867 multiline comments in "4-line" format (the rest)

So in conclusion: I'm not opposed to this patch, as long as someone converts
at least all 3-line comments to 4-line (including mine) and then the WARN can
become an ERROR.  If someone wants to do that, the checkpatch output from the
script at the top is probably a good start.  Otherwise the chosen format
should be the most common, considering that most maintainers have been using
it for 5 years or more.

If we want to accept both it's certainly fine by me, and then it's enough not to
warn at all on the leading line.  In that case only 289 files (baseline is 152)
would have zero errors and some warnings.  I think it's the best compromise.

I attach the incremental patch I used for the alternate rule.

Paolo

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 52ab18bfce..0f182670b8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1568,10 +1568,18 @@ sub process {
 
 # Block comment styles
 
-		# Block comments use /* on a line of its own
-		if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&	#inline /*...*/
-		    $rawline =~ m@^\+.*/\*\*?[ \t]*.+[ \t]*$@) { # /* or /** non-blank
-			WARN("Block comments use a leading /* on a separate line\n" . $herecurr);
+		# Doc comments use /** on a line of its own
+		if ($rawline =~ m@/\*\*@ && $rawline !~ m@^\+[ \t]*/\*\*$@) { # /* or /** non-blank
+			ERROR("Documentation comments use a leading /** on a separate line\n" . $herecurr);
+		}
+		if ($rawline =~ m@\*\*/@ && $rawline !~ m@^\+[ \t]*\*\*\/@) { # /* or /** non-blank
+			ERROR("Documentation comments use a trailing */ on a separate line\n" . $herecurr);
+		}
+
+		# Block comments use /* on the first line, except for the license header
+		if ($rawline !~ m@^\+.*/\*.*\*/[ \t]*$@ &&	#inline /*...*/
+		    $rawline =~ m@^\+.*/\*[ \t]*$@ && $realline > 1) { # /* or /** non-blank
+			WARN("Block comment with leading /* on a separate line\n" . $herecurr);
 		}
 
 # Block comments use * on subsequent lines