From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45137) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZYxJk-0001s1-HW for qemu-devel@nongnu.org; Mon, 07 Sep 2015 10:22:45 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZYxJf-0007mZ-Jr for qemu-devel@nongnu.org; Mon, 07 Sep 2015 10:22:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:47443) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZYxJf-0007mJ-CK for qemu-devel@nongnu.org; Mon, 07 Sep 2015 10:22:39 -0400 References: <1441619584-17992-1-git-send-email-pbonzini@redhat.com> <1441619584-17992-3-git-send-email-pbonzini@redhat.com> <55ED8EA0.6040400@redhat.com> From: Paolo Bonzini Message-ID: <55ED9DA9.80208@redhat.com> Date: Mon, 7 Sep 2015 16:22:33 +0200 MIME-Version: 1.0 In-Reply-To: <55ED8EA0.6040400@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH 2/4] CODING_STYLE, checkpatch: update line length rules List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Thomas Huth , qemu-devel@nongnu.org Cc: Markus Armbruster , Andreas Faerber , Eduardo Habkost 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 >> --- >> 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 l= ines. >> =20 >> 2. Line width >> =20 >> -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 subsyst= ems >> +that use long function or symbol names. Even in that case, do not ma= ke >> +lines much longer than 76 characters. >> =20 >> Rationale: >> - Some people like to tile their 24" screens with a 6x4 matrix of 80= x24 >> - 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 th= ree-way >> + diffs) on an 80-column terminal, accounting for two extra characte= rs. >> + 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. >> =20 >> 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)$/); >> =20 >> -#80 column limit >> - if ($line =3D~ /^\+/ && $prevrawline !~ /\/\*\*/ && >> - $rawline !~ /^.\s*\*\s*\@$Ident\s/ && >> +#90 column limit >> + if ($line =3D~ /^\+/ && >> !($line =3D~ /^\+\s*$logFunctions\s*\(\s*(?:(KERN_\S+\s*|[^"]*)= )?"[X\t]*"\s*(?:,|\)\s*;)\s*$/ || >> - $line =3D~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) && >> - $length > 80) >> + !($line =3D~ /^\+\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 =3D~ /^\+ \* /)) { >> + # The BSD license blurb has 80 character lines. >> + # Avoid warning on cut-and-pasted license text. >> + WARN("line over 76 characters\n" . $herecurr); >=20 > 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 8= 0 > 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 yo= u > have to reformat all patches that move code around - and it will also b= e > 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 lik= e > 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 " git show " 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