| Message ID | 20201105154208.12442-1-ganqixin@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | scripts/checkpatch.pl: Modify the line length limit of the code | expand |
On Fri, 6 Nov 2020 at 06:15, Gan Qixin <ganqixin@huawei.com> wrote: > > Modify the rule that limit the length of lines according to the following ideas: > > --add a variable max_line_length to indicate the limit of line length and set it to 100 by default > --when the line length exceeds max_line_length, output warning information instead of error > --if/while/etc brace do not go on next line whether the line length exceeds max_line_length or not > > Signed-off-by: Gan Qixin <ganqixin@huawei.com> > --- > scripts/checkpatch.pl | 18 +++++------------- > 1 file changed, 5 insertions(+), 13 deletions(-) For the code changes Reviewed-by: Peter Maydell <peter.maydell@linaro.org> but we also need to update our coding style documentation to match. I'll send out a patch with some proposed text. Side note: the kernel version of this checkpatch change (kernel commit bdc48fa11e46f867) suppresses all line-length warnings for the "--file" use case. Do we care about that? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 6 Nov 2020 at 06:15, Gan Qixin <ganqixin@huawei.com> wrote: >> >> Modify the rule that limit the length of lines according to the following ideas: >> >> --add a variable max_line_length to indicate the limit of line length and set it to 100 by default >> --when the line length exceeds max_line_length, output warning information instead of error >> --if/while/etc brace do not go on next line whether the line length exceeds max_line_length or not The commit message fails at explaining *why*. A controversial change like this should not be committed without proper rationale. >> Signed-off-by: Gan Qixin <ganqixin@huawei.com> >> --- >> scripts/checkpatch.pl | 18 +++++------------- >> 1 file changed, 5 insertions(+), 13 deletions(-) > > For the code changes > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > but we also need to update our coding style documentation > to match. I'll send out a patch with some proposed text. I disagree with the coding style change. The current "warn at 80, error at 90" is a compromise. It's the result of a lengthy argument. Why reopen it? > Side note: the kernel version of this checkpatch change > (kernel commit bdc48fa11e46f867) suppresses all line-length > warnings for the "--file" use case. Do we care about that? The kernel patch at least has a decent commit message.
On Fri, 6 Nov 2020 at 13:07, Markus Armbruster <armbru@redhat.com> wrote: > The current "warn at 80, error at 90" is a compromise. It's the result > of a lengthy argument. Why reopen it? There was some previous discussion under this thread: https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg05653.html which I think is what prompted this patch. thanks -- PMM
On 11/6/20 2:39 PM, Peter Maydell wrote: > On Fri, 6 Nov 2020 at 13:07, Markus Armbruster <armbru@redhat.com> wrote: >> The current "warn at 80, error at 90" is a compromise. It's the result >> of a lengthy argument. Why reopen it? > > There was some previous discussion under this thread: > https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg05653.html > > which I think is what prompted this patch. Can we keep the error please? Maybe 132 is the next display logical limit once we increased the warning from 80 to 100. I understand hardware evolved, we have larger displays with better resolution and can fit more characters in a line. I am a bit wary however functions become heavier (more code into a single function). Maybe this checkpatch change should go with a another one warning when a function has more than 80 lines, excluding comments? (Even 80 is too much for my taste). Regards, Phil.
On Fri, 6 Nov 2020 at 14:08, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > Can we keep the error please? Maybe 132 is the next display logical > limit once we increased the warning from 80 to 100. > > I understand hardware evolved, we have larger displays with better > resolution and can fit more characters in a line. > I am a bit wary however functions become heavier (more code into > a single function). Maybe this checkpatch change should go with > a another one warning when a function has more than 80 lines, > excluding comments? (Even 80 is too much for my taste). Personally I just don't think checkpatch should be nudging people into folding 85-character lines, especially when there are multiple very similar lines in a row and only one would get folded, eg the prototypes in target/arm/helper.h -- some of these just edge beyond 80 characters and I think wrapping them is clearly worse for readability. If we don't want people sending us "style fix" patches which wrap >80 char lines (which I think we do not) then we shouldn't have checkpatch complain about them, because if it does then that's what we get. thanks -- PMM
On 11/6/20 3:16 PM, Peter Maydell wrote: > On Fri, 6 Nov 2020 at 14:08, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> Can we keep the error please? Maybe 132 is the next display logical >> limit once we increased the warning from 80 to 100. >> >> I understand hardware evolved, we have larger displays with better >> resolution and can fit more characters in a line. >> I am a bit wary however functions become heavier (more code into >> a single function). Maybe this checkpatch change should go with >> a another one warning when a function has more than 80 lines, >> excluding comments? (Even 80 is too much for my taste). > > Personally I just don't think checkpatch should be nudging people > into folding 85-character lines, especially when there are > multiple very similar lines in a row and only one would get > folded, eg the prototypes in target/arm/helper.h -- some of > these just edge beyond 80 characters and I think wrapping them > is clearly worse for readability. If we don't want people > sending us "style fix" patches which wrap >80 char lines > (which I think we do not) then we shouldn't have checkpatch > complain about them, because if it does then that's what we get. I think I was not clear. I am not arguing against changing the *length* limit of a line (although I'd still keep one, as I don't think we want lines with 500 characters). I'm suggesting an orthogonal change, restricting the number of lines in a function :) > > thanks > -- PMM >
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 6 Nov 2020 at 14:08, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> Can we keep the error please? Maybe 132 is the next display logical >> limit once we increased the warning from 80 to 100. >> >> I understand hardware evolved, we have larger displays with better >> resolution and can fit more characters in a line. [...] > > Personally I just don't think checkpatch should be nudging people > into folding 85-character lines, especially when there are > multiple very similar lines in a row and only one would get > folded, eg the prototypes in target/arm/helper.h -- some of > these just edge beyond 80 characters and I think wrapping them > is clearly worse for readability. The warning's intent is "are you sure this line is better not broken?" The problem is people treating it as an order that absolves them from using good judgement instead. I propose to fix it by phrasing the warning more clearly. Instead of WARNING: line over 80 characters we could say WARNING: line over 80 characters Please examine the line, and use your judgement to decide whether it should be broken. > If we don't want people > sending us "style fix" patches which wrap >80 char lines > (which I think we do not) then we shouldn't have checkpatch > complain about them, because if it does then that's what we get. I think that's throwing out the baby with the bathwater. checkpatch's purpose is not guiding inexperienced developers to style issues they can fix. It is relieving maintainers of the tedium of catching and explaining certain kinds of issues patches frequently have. Neutering checks that have led inexperienced developers to post less than useful patches may well relieve maintainers of having to reject such patches. But it comes a price: maintainers and contributors lose automated help with checking useful patches. I consider that a bad trade. We may want to discourage inexperienced contributors from sending us style fix patches. Fixing style takes good taste, which develops only with experience. Moreover, fixing up style builds only little experience. At best, it exercises "configure; make check" and the patch submission process and running "make check"). There are better ways to get your feet wet.
On Fri, 6 Nov 2020 at 16:08, Markus Armbruster <armbru@redhat.com> wrote: > Peter Maydell <peter.maydell@linaro.org> writes: > > Personally I just don't think checkpatch should be nudging people > > into folding 85-character lines, especially when there are > > multiple very similar lines in a row and only one would get > > folded, eg the prototypes in target/arm/helper.h -- some of > > these just edge beyond 80 characters and I think wrapping them > > is clearly worse for readability. > > The warning's intent is "are you sure this line is better not broken?" > The problem is people treating it as an order that absolves them from > using good judgement instead. > > I propose to fix it by phrasing the warning more clearly. Instead of > > WARNING: line over 80 characters > > we could say > > WARNING: line over 80 characters > Please examine the line, and use your judgement to decide whether > it should be broken. I would suggest that for a line over 80 characters and less than 85 characters, the answer is going to be "better not broken" a pretty high percentage of the time; that is, the warning has too many false positives, and we should tune it to have fewer. And the lure of "produce no warnings" is a strong one, so we should be at least cautious about what our tooling is nudging us into doing. thanks -- PMM
> -----Original Message----- > From: Peter Maydell [mailto:peter.maydell@linaro.org] > Sent: Saturday, November 7, 2020 12:20 AM > To: Markus Armbruster <armbru@redhat.com> > Cc: Philippe Mathieu-Daudé <philmd@redhat.com>; Daniel P. Berrange > <berrange@redhat.com>; Zhanghailiang > <zhang.zhanghailiang@huawei.com>; Michael S. Tsirkin <mst@redhat.com>; > QEMU Trivial <qemu-trivial@nongnu.org>; QEMU Developers > <qemu-devel@nongnu.org>; ganqixin <ganqixin@huawei.com>; Paolo > Bonzini <pbonzini@redhat.com>; Chenqun (kuhn) > <kuhn.chenqun@huawei.com> > Subject: Re: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the > code > > On Fri, 6 Nov 2020 at 16:08, Markus Armbruster <armbru@redhat.com> > wrote: > > Peter Maydell <peter.maydell@linaro.org> writes: > > > Personally I just don't think checkpatch should be nudging people > > > into folding 85-character lines, especially when there are multiple > > > very similar lines in a row and only one would get folded, eg the > > > prototypes in target/arm/helper.h -- some of these just edge beyond > > > 80 characters and I think wrapping them is clearly worse for > > > readability. > > > > The warning's intent is "are you sure this line is better not broken?" > > The problem is people treating it as an order that absolves them from > > using good judgement instead. > > > > I propose to fix it by phrasing the warning more clearly. Instead of > > > > WARNING: line over 80 characters > > > > we could say > > > > WARNING: line over 80 characters > > Please examine the line, and use your judgement to decide whether > > it should be broken. > > I would suggest that for a line over 80 characters and less than > 85 characters, the answer is going to be "better not broken" > a pretty high percentage of the time; that is, the warning has too many false > positives, and we should tune it to have fewer. > > And the lure of "produce no warnings" is a strong one, so we should be at > least cautious about what our tooling is nudging us into doing. > Personally, I agree with this view. I think it is not a good thing to check for many false positives, whether for maintainers or inexperienced developers.The limit of 100 is set by referring to the kernel commit(bdc48fa11e46f867), maybe it's not very suitable here. Would it be more appropriate for us to set the limit to 85 or 90? In addition, could we tell developers in the warning message that this is just to remind them to pay attention and not to replace their judgment (like Markus said)? Thanks, Gan Qixin > thanks > -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Fri, 6 Nov 2020 at 16:08, Markus Armbruster <armbru@redhat.com> wrote: >> Peter Maydell <peter.maydell@linaro.org> writes: >> > Personally I just don't think checkpatch should be nudging people >> > into folding 85-character lines, especially when there are >> > multiple very similar lines in a row and only one would get >> > folded, eg the prototypes in target/arm/helper.h -- some of >> > these just edge beyond 80 characters and I think wrapping them >> > is clearly worse for readability. >> >> The warning's intent is "are you sure this line is better not broken?" >> The problem is people treating it as an order that absolves them from >> using good judgement instead. >> >> I propose to fix it by phrasing the warning more clearly. Instead of >> >> WARNING: line over 80 characters >> >> we could say >> >> WARNING: line over 80 characters >> Please examine the line, and use your judgement to decide whether >> it should be broken. > > I would suggest that for a line over 80 characters and less than > 85 characters, the answer is going to be "better not broken" > a pretty high percentage of the time; that is, the warning > has too many false positives, and we should tune it to have fewer. > > And the lure of "produce no warnings" is a strong one, so > we should be at least cautious about what our tooling is > nudging us into doing. CODING_STYLE.rst: "Lines should be 80 characters; try not to make them longer." I'd like to keep the tooling we have to help us with trying not to make them longer. If we have lost the ability to differentiate between "warning" and "error", call it something else.
On Mon, 9 Nov 2020 at 09:01, Markus Armbruster <armbru@redhat.com> wrote: > CODING_STYLE.rst: "Lines should be 80 characters; try not to make them > longer." I'd like to keep the tooling we have to help us with trying > not to make them longer. > > If we have lost the ability to differentiate between "warning" and > "error", call it something else. Personally I just want checkpatch with its default arguments not to complain about code that we'd be happy to accept in the tree. It's unnecessary noise when I write and check the code locally, when patchew runs on the patch on the list and then when it goes into a pullreq. Do we need a new "be really strict" option? thanks -- PMM
Peter Maydell <peter.maydell@linaro.org> writes: > On Mon, 9 Nov 2020 at 09:01, Markus Armbruster <armbru@redhat.com> wrote: >> CODING_STYLE.rst: "Lines should be 80 characters; try not to make them >> longer." I'd like to keep the tooling we have to help us with trying >> not to make them longer. >> >> If we have lost the ability to differentiate between "warning" and >> "error", call it something else. > > Personally I just want checkpatch with its default arguments not > to complain about code that we'd be happy to accept in the tree. This means losing complaints about code we don't want to accept, because there is a grey area where checkpatch can't be sure. CODING_STYLE.rst demands "try not to make lines longer than 80 characters, but if you decide you need to, don't make them longer than 90". As long as we have this grey area where we want developers to try, checkpatch's job is to remind them to try. > It's unnecessary noise when I write and check the code locally, > when patchew runs on the patch on the list and then when it goes > into a pullreq. Checking locally: noise or not is up to you. Patchew: no, it's not noise here. Patch review is exactly where the reminder is needed. Pull request: assuming patch review did its job, all that's left is noise. > Do we need a new "be really strict" option? No objection, as long as we stick to strict for patch review.
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 88c858f67c..84a72d47ad 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -35,6 +35,7 @@ my $summary_file = 0; my $root; my %debug; my $help = 0; +my $max_line_length = 100; sub help { my ($exitcode) = @_; @@ -1628,17 +1629,13 @@ sub process { # check we are in a valid source file if not then ignore this hunk next if ($realfile !~ /$SrcFile/); -#90 column limit; exempt URLs, if no other words on line +#$max_line_length column limit; exempt URLs, if no other words on line if ($line =~ /^\+/ && !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) && !($rawline =~ /^[^[:alnum:]]*https?:\S*$/) && - $length > 80) + $length > $max_line_length) { - if ($length > 90) { - ERROR("line over 90 characters\n" . $herecurr); - } else { - WARN("line over 80 characters\n" . $herecurr); - } + WARN("line over $max_line_length characters\n" . $herecurr); } # check for spaces before a quoted newline @@ -1831,13 +1828,8 @@ sub process { #print "realcnt<$realcnt> ctx_cnt<$ctx_cnt>\n"; #print "pre<$pre_ctx>\nline<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>\n"; - # The length of the "previous line" is checked against 80 because it - # includes the + at the beginning of the line (if the actual line has - # 79 or 80 characters, it is no longer possible to add a space and an - # opening brace there) if ($#ctx == 0 && $ctx !~ /{\s*/ && - defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*\{/ && - defined($lines[$ctx_ln - 2]) && length($lines[$ctx_ln - 2]) < 80) { + defined($lines[$ctx_ln - 1]) && $lines[$ctx_ln - 1] =~ /^\+\s*\{/) { ERROR("that open brace { should be on the previous line\n" . "$here\n$ctx\n$rawlines[$ctx_ln - 1]\n"); }
Modify the rule that limit the length of lines according to the following ideas: --add a variable max_line_length to indicate the limit of line length and set it to 100 by default --when the line length exceeds max_line_length, output warning information instead of error --if/while/etc brace do not go on next line whether the line length exceeds max_line_length or not Signed-off-by: Gan Qixin <ganqixin@huawei.com> --- scripts/checkpatch.pl | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-)