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

[Markus Armbruster]

Thomas Huth <address@hidden> writes:

> On 07/09/15 16:22, Paolo Bonzini wrote:
>> 
>> 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.
> [...]
>>> 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.

Feature.  Nudging developers to tidy up style when they touch code is
what we've always done.  You're not entitled to be able to copy existing
code unchanged.

Now from a reviewer's point of view: I can't recall a review where style
cleanup caused me trouble, *except* when mixed up with other changes.
But that's an orthogonal no-no.

> [...]
>> 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.
>
> Apart from copy-n-pasting, there is also the problem that you can run
> "checkpatch.pl -f" on a whole file ... it would also be ugly to suddenly
> have (much) more warnings here.

Feature.  If you run checkpatch on a whole file, you obviously do it to
find its ugly spots.  Lines longer than 76 characters qualify.

>> 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.
>
> 79 sounds at least better to me than 76. So if you really want to change
> the limit, please go for 79 instead of 76.

Have you read the thread leading up to this patch?