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(-)

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");
 			}
-- 
2.23.0

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

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.

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