Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules

[Paolo Bonzini]

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 <address@hidden>
>> ---
>>  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 !~ /address@hidden/ &&
>> +#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