| Message ID | 1441619584-17992-3-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <pbonzini@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. 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
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 <pbonzini@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
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 <pbonzini@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. [...] >> 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. [...] > 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. > 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. Thomas
Paolo Bonzini <pbonzini@redhat.com> writes: > 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 <pbonzini@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. Why not simply reflow all the offending license blurbs? Want me to prep such a patch? > + WARN("line over 76 characters\n" . $herecurr); > + } elsif ($length > 80) { > + # Do not confuse the user by introducing yet > + # another limit (80 characters). Technically, > + # this line *is* over 76 characters. > + WARN("line over 76 characters\n" . $herecurr); > + } > } > > # check for spaces before a quoted newline
Thomas Huth <thuth@redhat.com> 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 <pbonzini@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. > [...] >>> 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?
On 07/09/2015 17:17, Markus Armbruster wrote: >> > + } elsif ($length > 76 && !($rawline =~ /^\+ \* /)) { >> > + # The BSD license blurb has 80 character lines. >> > + # Avoid warning on cut-and-pasted license text. > Why not simply reflow all the offending license blurbs? > > Want me to prep such a patch? Actually there are other license blurbs with 80-character lines. It's just that for the GPL we just refer to the COPYING file most of the time, instead of TYPING SEVERAL LINES ABOUT MERCHANTABILITY, whatever it is. Paolo
On 07/09/2015 17:23, Markus Armbruster wrote: > > 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 statistics, half of QEMU's files has at least one 76-79 character line. The noise from checkpatch.pl -f is actually a worse thing than the cut-and-paste, but that's something that can be fixed in other ways (e.g. different strictness for checkpatch.pl vs. checkpatch.pl -f). That said, and even though Thomas obviously hasn't read the previous discussion, :) I do believe that 76 characters is too strict a limit. 76 would be great (two levels of email quoting are what you get 99% of the time), and 78 would be nice, but I believe 79 provides the biggest bang for the buck. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 07/09/2015 17:17, Markus Armbruster wrote: >>> > + } elsif ($length > 76 && !($rawline =~ /^\+ \* /)) { >>> > + # The BSD license blurb has 80 character lines. >>> > + # Avoid warning on cut-and-pasted license text. >> Why not simply reflow all the offending license blurbs? >> >> Want me to prep such a patch? > > Actually there are other license blurbs with 80-character lines. It's > just that for the GPL we just refer to the COPYING file most of the > time, instead of TYPING SEVERAL LINES ABOUT MERCHANTABILITY, whatever it is. My offer to "reflow all the offending license blurbs" includes all of them.
On 07/09/15 17:23, Markus Armbruster wrote: > Thomas Huth <thuth@redhat.com> writes: > >> On 07/09/15 16:22, Paolo Bonzini wrote: >> [...] >>> 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. I sometimes also use "checkpatch.pl -f" to check my own new code that I've added to files (when I did not create patch files yet). When it suddenly spams me because of such new line-length warnings, that would be quite annoying, I think. >>> 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? You mean the v1 thread of this patch? I slightly recall it ... and just looked it up again. As far as I can see, you were the only one who suggested such a small limit, John wanted to keep it at 80 columns and Andreas suggested 78. I can see your point with "typographic style suggests 60 characters", but I think you can not really fully apply this to source code - it reads much different that natural text, e.g. there are much more separating special characters like parentheses and commas in between. We could maybe mention that 76 columns limit in the coding style document, to make people aware that they should start thinking about breaking lines in two when they reach 76 characters in a line. But changing checkpatch.pl to use this limit really sounds too harsh to me. Maybe 79, yes, since that really gives to the possibility to nicely view patches in a 80 columns terminal, too. (But if you try to view a 2-way patch in a 80-columns window, you quite masochistic anyway, so I don't see the point in reducing it to 78). Less than 79 would IMHO cause too much noise with the already existing code, so I really would appreciate if checkpatch.pl would not use less than that. Thomas
Paolo Bonzini <pbonzini@redhat.com> writes: > On 07/09/2015 17:23, Markus Armbruster wrote: >> > 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 statistics, half of QEMU's files has at least one 76-79 > character line. The noise from checkpatch.pl -f is actually a worse > thing than the cut-and-paste, but that's something that can be fixed in > other ways (e.g. different strictness for checkpatch.pl vs. > checkpatch.pl -f). Yup. > That said, and even though Thomas obviously hasn't read the previous > discussion, :) I do believe that 76 characters is too strict a limit. It's not a strict limit, it's a warning. The strict limit is 90. > 76 would be great (two levels of email quoting are what you get 99% of > the time), and 78 would be nice, but I believe 79 provides the biggest > bang for the buck. 78 gives one level of quoting, and two-way diffs.
On 09/07/2015 01:05 PM, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 07/09/2015 17:23, Markus Armbruster wrote: >>>> 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 statistics, half of QEMU's files has at least one 76-79 >> character line. The noise from checkpatch.pl -f is actually a worse >> thing than the cut-and-paste, but that's something that can be fixed in >> other ways (e.g. different strictness for checkpatch.pl vs. >> checkpatch.pl -f). > > Yup. > >> That said, and even though Thomas obviously hasn't read the previous >> discussion, :) I do believe that 76 characters is too strict a limit. > > It's not a strict limit, it's a warning. The strict limit is 90. > The problem is that checkpatch returns non-zero for warnings, so this interrupts my workflow; this means that many of my "patch testing" scripts will now "fail" due to the shortened limit. Unless there are documented error codes for checkpatch -- e.g. {0: fine, 1: warnings, 2: errors, 3+: script-errors } I could then update my scripts to tolerate warnings, but this still seems like a pain to me. >> 76 would be great (two levels of email quoting are what you get 99% of >> the time), and 78 would be nice, but I believe 79 provides the biggest >> bang for the buck. > > 78 gives one level of quoting, and two-way diffs. >
On 7 September 2015 at 18:05, Markus Armbruster <armbru@redhat.com> wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: >> That said, and even though Thomas obviously hasn't read the previous >> discussion, :) I do believe that 76 characters is too strict a limit. > > It's not a strict limit, it's a warning. The strict limit is 90. I tend to bounce patches on review for checkpatch warnings... I don't make much distinction between a warning and an error. thanks -- PMM
On Mon, Sep 07, 2015 at 07:02:55PM +0200, Thomas Huth wrote: [...] > We could maybe mention that 76 columns limit in the coding style > document, to make people aware that they should start thinking about > breaking lines in two when they reach 76 characters in a line. But > changing checkpatch.pl to use this limit really sounds too harsh to me. Please don't. The whole point of checkpatch.pl is to let a robot do the work of checking coding style instead of relying on us humans.
On Mon, Sep 07, 2015 at 06:06:25PM +0200, Paolo Bonzini wrote: > On 07/09/2015 17:23, Markus Armbruster wrote: > > > 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 statistics, half of QEMU's files has at least one 76-79 > character line. The noise from checkpatch.pl -f is actually a worse > thing than the cut-and-paste, but that's something that can be fixed in > other ways (e.g. different strictness for checkpatch.pl vs. > checkpatch.pl -f). Why exactly it's considered noise? I fail to see why would somebody use checkpatch.pl -f if they don't want to see all warnings about the whole file (including the lines that they didn't write). If somebody doesn't want to see them, they can simply run checkpatch.pl on the diff instead of using -f.
On Wed, Sep 09, 2015 at 06:22:15PM +0100, Peter Maydell wrote: > On 7 September 2015 at 18:05, Markus Armbruster <armbru@redhat.com> wrote: > > Paolo Bonzini <pbonzini@redhat.com> writes: > >> That said, and even though Thomas obviously hasn't read the previous > >> discussion, :) I do believe that 76 characters is too strict a limit. > > > > It's not a strict limit, it's a warning. The strict limit is 90. > > I tend to bounce patches on review for checkpatch warnings... > I don't make much distinction between a warning and an error. If there are existing warnings that will make you bounce patches automatically, shouldn't they become errors? I believe there are things we want to warn people about on checkpatch.pl because they should be discouraged (like lines a bit longer than 76^H^H78^H^H79 columns), but that shouldn't make a patch be automatically rejected.
On 09/09/2015 19:22, Peter Maydell wrote: > On 7 September 2015 at 18:05, Markus Armbruster <armbru@redhat.com> wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >>> That said, and even though Thomas obviously hasn't read the previous >>> discussion, :) I do believe that 76 characters is too strict a limit. >> >> It's not a strict limit, it's a warning. The strict limit is 90. > > I tend to bounce patches on review for checkpatch warnings... > I don't make much distinction between a warning and an error. Doing something about this is the point of this series. As a start, it enables a few more warnings (patch 3) so that maintainers start seeing them and get used to them. Some may reject patches altogether based on the warnings, some may not. With just a few exceptions (tabs, line lengths, braces) it makes a lot of sense to fix warnings in the whole tree with a single sweeping change. If we agree about this, we can upgrade warnings to error at the time of the fix, or even before. Ultimately, warnings should be used only for things that are _really_ subjective, so that we can aim at an error-free (but not warning-free) "checkpatch -f". Paolo
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); + } elsif ($length > 80) { + # Do not confuse the user by introducing yet + # another limit (80 characters). Technically, + # this line *is* over 76 characters. + WARN("line over 76 characters\n" . $herecurr); + } } # check for spaces before a quoted newline
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 <pbonzini@redhat.com> --- CODING_STYLE | 13 ++++++++++--- scripts/checkpatch.pl | 21 +++++++++++++++------ 2 files changed, 25 insertions(+), 9 deletions(-)