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

[Thomas Huth]

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.

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.

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...

 Thomas