| Message ID | 1434698944-3331-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, 19 Jun 2015 09:29:04 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > 1) 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. > > 2) Mixed declarations also do exist at the top of #ifdef blocks. > Remark on this particular usage and suggest an alternative. > > Cc: Andreas Faerber <afaerber@suse.de> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > CODING_STYLE | 21 ++++++++++++++++----- > scripts/checkpatch.pl | 9 ++++++--- > 2 files changed, 22 insertions(+), 8 deletions(-) > > diff --git a/CODING_STYLE b/CODING_STYLE > index d46cfa5..d013cb8 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -31,7 +31,11 @@ Do not leave whitespace dangling off the ends of lines. > > 2. Line width > > -Lines are 80 characters; not longer. > +Lines should be 80 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 80 characters. Good idea ... this very, very strict 80 characters limit often drove me crazy already. If the code is more readable with a 81 or 82 characters line, that's IMHO a much better way to write code than to break it artificially just to satisfy that rule. ... > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl > index 7f0aae9..f4e7050 100755 > --- a/scripts/checkpatch.pl > +++ b/scripts/checkpatch.pl > @@ -1470,10 +1470,13 @@ sub process { > if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ && > $rawline !~ /^.\s*\*\s*\@$Ident\s/ && > !($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); > + } if ($length > 80) { Did you mean to use "elsif" here instead (because you've put the if on the same line as the "}")? > + WARN("line over 80 characters\n" . $herecurr); > + } > } > Thomas
On 19/06/2015 09:53, Thomas Huth wrote: > > - WARN("line over 80 characters\n" . $herecurr); > > + if ($length > 90) { > > + ERROR("line over 90 characters\n" . $herecurr); > > + } if ($length > 80) { > > Did you mean to use "elsif" here instead (because you've put the if on > the same line as the "}")? Yes, and this patch was really meant as little more than RFC. Paolo
Am 19.06.2015 um 09:29 schrieb Paolo Bonzini: > 1) 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. > > 2) Mixed declarations also do exist at the top of #ifdef blocks. > Remark on this particular usage and suggest an alternative. > > Cc: Andreas Faerber <afaerber@suse.de> > Cc: Markus Armbruster <armbru@redhat.com> > Cc: Eduardo Habkost <ehabkost@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > CODING_STYLE | 21 ++++++++++++++++----- > scripts/checkpatch.pl | 9 ++++++--- > 2 files changed, 22 insertions(+), 8 deletions(-) > > diff --git a/CODING_STYLE b/CODING_STYLE > index d46cfa5..d013cb8 100644 > --- a/CODING_STYLE > +++ b/CODING_STYLE > @@ -31,7 +31,11 @@ Do not leave whitespace dangling off the ends of lines. > > 2. Line width > > -Lines are 80 characters; not longer. > +Lines should be 80 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 80 characters. Anthony had always allowed sensible exceptions to that rule, so +1 for reformulating it here. However, I would suggest that in that case we should lower the recommendation/warning to 78 chars, with the rationale of not only the actual code but also two-way diffs (79 chars plus +/-/space) and three-way diffs (78 chars plus 2x +/-/space) fitting into standard 80x24 windows. Either way, can you please decouple the two changes? Regards, Andreas > > Rationale: > - Some people like to tile their 24" screens with a 6x4 matrix of 80x24 > @@ -39,6 +43,8 @@ Rationale: > 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 [snip]
On 19/06/2015 10:09, Andreas Färber wrote: >> > -Lines are 80 characters; not longer. >> > +Lines should be 80 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 80 characters. > Anthony had always allowed sensible exceptions to that rule, so +1 for > reformulating it here. > > However, I would suggest that in that case we should lower the > recommendation/warning to 78 chars, with the rationale of not only the > actual code but also two-way diffs (79 chars plus ±/space) and > three-way diffs (78 chars plus 2x ±/space) fitting into standard 80x24 > windows. Good idea. > Either way, can you please decouple the two changes? Sure, didn't want to spam people with a series on what is mostly an RFC. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 19/06/2015 10:09, Andreas Färber wrote: >>> > -Lines are 80 characters; not longer. >>> > +Lines should be 80 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 80 characters. >> Anthony had always allowed sensible exceptions to that rule, so +1 for >> reformulating it here. >> >> However, I would suggest that in that case we should lower the >> recommendation/warning to 78 chars, with the rationale of not only the >> actual code but also two-way diffs (79 chars plus ±/space) and >> three-way diffs (78 chars plus 2x ±/space) fitting into standard 80x24 >> windows. > > Good idea. I personally prefer a slightly lower limit, to keep quoted patches in e-mail neatly under 80. How much writability to trade for readability is subjective, and I won't argue against 78. >> Either way, can you please decouple the two changes? > > Sure, didn't want to spam people with a series on what is mostly an RFC. Looking forward to your respin.
On 08/25/2015 02:20 PM, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 19/06/2015 10:09, Andreas Färber wrote: >>>>> -Lines are 80 characters; not longer. >>>>> +Lines should be 80 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 80 characters. >>> Anthony had always allowed sensible exceptions to that rule, so +1 for >>> reformulating it here. >>> >>> However, I would suggest that in that case we should lower the >>> recommendation/warning to 78 chars, with the rationale of not only the >>> actual code but also two-way diffs (79 chars plus ±/space) and >>> three-way diffs (78 chars plus 2x ±/space) fitting into standard 80x24 >>> windows. >> >> Good idea. > > I personally prefer a slightly lower limit, to keep quoted patches in > e-mail neatly under 80. How much writability to trade for readability > is subjective, and I won't argue against 78. > As long as we don't update the checkpatch tool to whine about this, since it might break a good amount of existing context, and this will just inconvenience everyone and provide no real benefit. Maybe if you can have it warn only for NEW lines and not for context, but if that's not possible I'm against shortening the existing limit. IMO: The 80 char width rule makes good sense, but forcing the margins even smaller on today's clearly-no-longer-a-terminal devices is just a little too much. Of course, gently prodding people to reconsider their line length if they are reliably approaching 75+ is another issue entirely... it just shouldn't reflect as non-success via checkpatch. >>> Either way, can you please decouple the two changes? >> >> Sure, didn't want to spam people with a series on what is mostly an RFC. > > Looking forward to your respin. >
John Snow <jsnow@redhat.com> writes: > On 08/25/2015 02:20 PM, Markus Armbruster wrote: >> Paolo Bonzini <pbonzini@redhat.com> writes: >> >>> On 19/06/2015 10:09, Andreas Färber wrote: >>>>>> -Lines are 80 characters; not longer. >>>>>> +Lines should be 80 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 80 characters. >>>> Anthony had always allowed sensible exceptions to that rule, so +1 for >>>> reformulating it here. >>>> >>>> However, I would suggest that in that case we should lower the >>>> recommendation/warning to 78 chars, with the rationale of not only the >>>> actual code but also two-way diffs (79 chars plus ±/space) and >>>> three-way diffs (78 chars plus 2x ±/space) fitting into standard 80x24 >>>> windows. >>> >>> Good idea. >> >> I personally prefer a slightly lower limit, to keep quoted patches in >> e-mail neatly under 80. How much writability to trade for readability >> is subjective, and I won't argue against 78. >> > > As long as we don't update the checkpatch tool to whine about this, > since it might break a good amount of existing context, and this will > just inconvenience everyone and provide no real benefit. > > Maybe if you can have it warn only for NEW lines and not for context, > but if that's not possible I'm against shortening the existing limit. > > IMO: The 80 char width rule makes good sense, but forcing the margins > even smaller on today's clearly-no-longer-a-terminal devices is just a > little too much. The reason for line length limit isn't antiquated hardware or antiquated habits, it's us antiquated human beings: we tend to have trouble following long lines with our eyes (I sure do). Typographic manuals suggest to limit columns to roughly 60 characters for exactly that reason[*]. Four levels of indentation plus 60 characters of actual text yields 76. > Of course, gently prodding people to reconsider their line length if > they are reliably approaching 75+ is another issue entirely... it just > shouldn't reflect as non-success via checkpatch. I think that's what Paolo's patch does. Namely: line shorter than the soft limit: fine line between soft and hard limit: warn line longer than the hard limit: error He kept the soft limit at 80, and set the hard limit to 90. Andreas suggested to lower the soft limit a bit, and I suggested to lower it a bit more. [...] [*] https://en.wikipedia.org/wiki/Column_(typography)#Typographic_style
diff --git a/CODING_STYLE b/CODING_STYLE index d46cfa5..d013cb8 100644 --- a/CODING_STYLE +++ b/CODING_STYLE @@ -31,7 +31,11 @@ Do not leave whitespace dangling off the ends of lines. 2. Line width -Lines are 80 characters; not longer. +Lines should be 80 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 80 characters. Rationale: - Some people like to tile their 24" screens with a 6x4 matrix of 80x24 @@ -39,6 +43,8 @@ Rationale: 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 @@ -87,10 +93,15 @@ Furthermore, it is the QEMU coding style. 5. Declarations -Mixed declarations (interleaving statements and declarations within blocks) -are not allowed; declarations should be at the beginning of blocks. In other -words, the code should not generate warnings if using GCC's --Wdeclaration-after-statement option. +Mixed declarations (interleaving statements and declarations within +blocks) are generally not allowed; declarations should be at the beginning +of blocks. + +Every now and then, an exception is made for declarations inside a +#ifdef or #ifndef block: if the code looks nicer, such declarations can +be placed at the top of the block even if there are statements above. +On the other hand, however, it's often best to move that #ifdef/#ifndef +block to a separate function altogether. 6. Conditional statements diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 7f0aae9..f4e7050 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -1470,10 +1470,13 @@ sub process { if ($line =~ /^\+/ && $prevrawline !~ /\/\*\*/ && $rawline !~ /^.\s*\*\s*\@$Ident\s/ && !($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); + } if ($length > 80) { + WARN("line over 80 characters\n" . $herecurr); + } } # check for spaces before a quoted newline
1) 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. 2) Mixed declarations also do exist at the top of #ifdef blocks. Remark on this particular usage and suggest an alternative. Cc: Andreas Faerber <afaerber@suse.de> Cc: Markus Armbruster <armbru@redhat.com> Cc: Eduardo Habkost <ehabkost@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- CODING_STYLE | 21 ++++++++++++++++----- scripts/checkpatch.pl | 9 ++++++--- 2 files changed, 22 insertions(+), 8 deletions(-)