On 07/09/2015 15:18, Thomas Huth wrote:
> On 07/09/15 11:53, Paolo Bonzini wrote:
>> Line lengths above 80 characters do exist. They are rare, but
>> they happen from time to time. An ignored rule is worse than an
>> exception to the rule, so do the latter.
>>
>> Based on remarks from the list, make the preferred line length
>> slightly lower than 80 characters, to account for extra characters
>> in unified diffs (including three-way diffs) and for email quoting.
>>
>> Checkpatch has some code to detect doc comments that doesn't apply
>> to QEMU; the usual limits apply even for doc comments in our case.
>>
>> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
>> ---
>> CODING_STYLE | 13 ++++++++++---
>> scripts/checkpatch.pl | 21 +++++++++++++++------
>> 2 files changed, 25 insertions(+), 9 deletions(-)
>>
>> diff --git a/CODING_STYLE b/CODING_STYLE
>> index 3c6978f..34d5526 100644
>> --- a/CODING_STYLE
>> +++ b/CODING_STYLE
>> @@ -31,14 +31,21 @@ Do not leave whitespace dangling off the ends of lines.
>>
>> 2. Line width
>>
>> -Lines are 80 characters; not longer.
>> +Lines should be 76 characters; try not to make them longer.
>> +
>> +Sometimes it is hard to do, especially when dealing with QEMU subsystems
>> +that use long function or symbol names. Even in that case, do not make
>> +lines much longer than 76 characters.
>>
>> Rationale:
>> - Some people like to tile their 24" screens with a 6x4 matrix of 80x24
>> - xterms and use vi in all of them. The best way to punish them is to
>> - let them keep doing it.
>> + xterms and use vi in all of them. They also examine diffs (and three-way
>> + diffs) on an 80-column terminal, accounting for two extra characters.
>> + The best way to punish them is to let them keep doing it.
>> - Code and especially patches is much more readable if limited to a sane
>> line length. Eighty is traditional.
>> + - The four-space indentation makes the most common excuse ("But look
>> + at all that white space on the left!") moot.
>> - It is the QEMU coding style.
>>
>> 3. Naming
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index 7f0aae9..bc32d8f 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1466,14 +1466,23 @@ sub process {
>> # check we are in a valid source file if not then ignore this hunk
>> next if ($realfile !~ /\.(h|c|cpp|s|S|pl|sh)$/);
>>
>> -#80 column limit
>> - if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ &&
>> - $rawline !~ /^.\s*\*\s*\@$Ident\s/ &&
>> +#90 column limit
>> + if ($line =~ /^\+/ &&
>> !($line =~
>> /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*))?"[X\t]*"\s*(?:,|\)\s*;)\s*$/
>> ||
>> - $line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
>> - $length > 80)
>> + !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/))
>> {
>> - WARN("line over 80 characters\n" . $herecurr);
>> + if ($length > 90) {
>> + ERROR("line over 90 characters\n" . $herecurr);
>> + } elsif ($length > 76 && !($rawline =~ /^\+ \* /)) {
>> + # The BSD license blurb has 80 character lines.
>> + # Avoid warning on cut-and-pasted license text.
>> + WARN("line over 76 characters\n" . $herecurr);
>
> Sorry, this is _ugly_! First, the limit in QEMU has always been 80
> columns in the past, thus we've got tons of code that is written with 80
> columns already, so you suddenly even can not even copy-n-paste / move
> around code anymore that was valid before?!? That'll be a PITA since you
> have to reformat all patches that move code around - and it will also be
> a PITA for all reviewers since checking whether old code matches new
> code becomes more difficult.
QEMU has 2816 files.
529 files have at least one line with 81+ columns.
1342 files have at least one 77-80 columns line that would now warn.
1474 files have no lines with 77+ columns.
Some classification of line lengths (with the "BSD exception"):
Length Count
77-80 16358
78-80 11853
79-80 7852
80 2623
81+ 6588
Some other interesting data:
- However, only 605 files have 5 or more lines with 77+ columns, so the
odds of warnings after copy-n-paste are pretty slim.
- Also, of the 77+ column lines, about 10% are in macros that are
aligned right with a backslash.
- 1195 files have at least one 78-80 columns that would now warn; 979
files have at least one 79-80 columns that would now warn; 644 files
have at least one 80 columns that would now warn.
- With the above patch, the "BSD exception" hits 5471 times, of which
about 1900 are false positives. If the warning is restricted to 80+
columns, the "BSD exception" is still necessary, but it only hits 1033
times and only 152 are false positives.
Based on the above data, I suggest having 79 columns as the limit (with
the "BSD exception" unfortunately). This still makes it possible to
watch diffs on an 80-column terminal without wrapping.
Alternatively you would have 80 columns with no "BSD exception", which
is (more or less) what was in the previous submission of the patch.
> Second, I really don't like the idea of introducing exceptions here like
> it is done with the BSD license check above ... that will only cause
> confusion and "let's add another exception" situations later, I think.
I agree it's not nice.
> Can't we simply keep the 80 columns limit instead, please? IMHO people
> having trouble to view patches with 80 columns should simply be forced
> to switch to proper tools instead...
It's just the default size of a terminal. If I do "<Ctrl-N> git show
<Right click> <Enter>" I get an 80-column terminal.
Paolo
ps: to find "BSD exception" false positives I used the following regex:
copy|deal|rights|included in|EXPRESS OR|OR OTHER|ARISING FROM,|at
your|or later.|by the Free|software without|THE USE OF
THIS|licenses/>.|its contributors|detailed
below.|USA.|USA|PURPOSE|CONSEQUENTIAL|STRICT
