The QEMU documentation mentions that lines should be up to 80
characters and that the script checkpatch will warn at 100 characters,
but the script warns at 80 characters and throw and error at 90, so
this commit changes to warn at 100.

As to why extend, the argument that resulted in the change of the
docs was that trying to always wrap to 80 columns could result in less
readable code, so sometimes not wrapping was the better choice and in
those circumstances checkpatch could nudge people into creating less
readable code.

A 132 error limit is put to catch overly big lines.

Based-on: 20201105154208.12442-1-ganqixin@huawei.com
Signed-off-by: Lucas Mateus Castro(alqotel) <lucas.araujo@eldorado.org.br>
---
Currently there's a disagreement between the checkpatch code and the
documentation, this RFC just changes the checkpatch to match the
documentation.
But there was a discussion in 2020 as the best way to deal with this,
some alternatives mentioned are: change the warning to remind people to
not blindly wrap just because of the warning, change to warn at 90 columns
(which would mean changing the column limit for the error as well). If any
of those are preferred I'll send a next version updating the documentation
as well as changing checkpatch.pl to the preferred behavior.
---
 scripts/checkpatch.pl | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index d900d18048..2c2d7b31ab 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1639,12 +1639,12 @@ sub process {
 		if ($line =~ /^\+/ &&
 		    !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
 		    !($rawline =~ /^[^[:alnum:]]*https?:\S*$/) &&
-		    $length > 80)
+		    $length > 100)
 		{
-			if ($length > 90) {
-				ERROR("line over 90 characters\n" . $herecurr);
+			if ($length > 132) {
+				ERROR("line over 132 characters\n" . $herecurr);
 			} else {
-				WARN("line over 80 characters\n" . $herecurr);
+				WARN("line over 100 characters\n" . $herecurr);
 			}
 		}
 
@@ -1838,13 +1838,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.25.1

On Mon, 6 Jun 2022 at 15:34, Lucas Mateus Castro(alqotel)
<lucas.araujo@eldorado.org.br> wrote:
>
> The QEMU documentation mentions that lines should be up to 80
> characters and that the script checkpatch will warn at 100 characters,
> but the script warns at 80 characters and throw and error at 90, so
> this commit changes to warn at 100.
>
> As to why extend, the argument that resulted in the change of the
> docs was that trying to always wrap to 80 columns could result in less
> readable code, so sometimes not wrapping was the better choice and in
> those circumstances checkpatch could nudge people into creating less
> readable code.
>
> A 132 error limit is put to catch overly big lines.
>
> Based-on: 20201105154208.12442-1-ganqixin@huawei.com
> Signed-off-by: Lucas Mateus Castro(alqotel) <lucas.araujo@eldorado.org.br>
> ---
> Currently there's a disagreement between the checkpatch code and the
> documentation, this RFC just changes the checkpatch to match the
> documentation.
> But there was a discussion in 2020 as the best way to deal with this,
> some alternatives mentioned are: change the warning to remind people to
> not blindly wrap just because of the warning, change to warn at 90 columns
> (which would mean changing the column limit for the error as well). If any
> of those are preferred I'll send a next version updating the documentation
> as well as changing checkpatch.pl to the preferred behavior.

The reason the code doesn't match the style docs is partly
my fault I guess. The style docs were updated with
commit a998de0dcd ("CODING_STYLE.rst: Be less strict about 80 character limit");
that commit message says "this goes with the checkpatch changes to warn at
100 characters rather than 80", but we never committed the checkpatch
changes. Those were this patch:
https://lore.kernel.org/qemu-devel/20201105154208.12442-1-ganqixin@huawei.com/
(which started the discussion out on an awkward footing by not including
the rationale in its commit message; it was prompted by a discussion on
a previous patchset:
https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg05653.html

Equivalent kernel checkpatch.pl loosening:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144

Anyway, I think the main objector last time around was Markus, and
there's kind of an unresolvable difference of views here:

(1) I want checkpatch not to warn about line lengths that in fact
we'd be happy to include in the tree (because it's noise, and it
pushes people into wrapping the cases which the style guide describes
as "awkwardly wrapped" and better not wrapped, to silence the warning),
so I don't want checkpatch to even warn on less than 90 or 100 chars.
This is effectively also the position that the Linux kernel
checkpatch takes these days.

(2) Markus (as I understand his point of view from the 2020 thread)
wants checkpatch to warn about anything over the "this is definitely
fine" 80 column mark, so that developers are reminded that
they might want to make a judgement call there.

The coding-style text reflects my point of view, because I
wrote it, and therefore personally I'm happy to update checkpatch
to match it :-)

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index d900d18048..2c2d7b31ab 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1639,12 +1639,12 @@ sub process {
>                 if ($line =~ /^\+/ &&
>                     !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
>                     !($rawline =~ /^[^[:alnum:]]*https?:\S*$/) &&
> -                   $length > 80)
> +                   $length > 100)

Gan Qixin's patch has the advantage of putting the max length into
a variable rather than continuing to hardcode it.

thanks
-- PMM

On 09/06/2022 10:56, Peter Maydell wrote:
>
> On Mon, 6 Jun 2022 at 15:34, Lucas Mateus Castro(alqotel)
> <lucas.araujo@eldorado.org.br>  wrote:
>> ---
>> Currently there's a disagreement between the checkpatch code and the
>> documentation, this RFC just changes the checkpatch to match the
>> documentation.
>> But there was a discussion in 2020 as the best way to deal with this,
>> some alternatives mentioned are: change the warning to remind people to
>> not blindly wrap just because of the warning, change to warn at 90 columns
>> (which would mean changing the column limit for the error as well). If any
>> of those are preferred I'll send a next version updating the documentation
>> as well as changing checkpatch.pl to the preferred behavior.
> The reason the code doesn't match the style docs is partly
> my fault I guess. The style docs were updated with
> commit a998de0dcd ("CODING_STYLE.rst: Be less strict about 80 character limit");
> that commit message says "this goes with the checkpatch changes to warn at
> 100 characters rather than 80", but we never committed the checkpatch
> changes. Those were this patch:
> https://lore.kernel.org/qemu-devel/20201105154208.12442-1-ganqixin@huawei.com/
> (which started the discussion out on an awkward footing by not including
> the rationale in its commit message; it was prompted by a discussion on
> a previous patchset:
> https://lists.gnu.org/archive/html/qemu-devel/2020-10/msg05653.html
>
> Equivalent kernel checkpatch.pl loosening:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144
Sorry I should've added those links to the commit message to update 
anyone getting in this discussion now, thanks for adding them.
>
> Anyway, I think the main objector last time around was Markus, and
> there's kind of an unresolvable difference of views here

My main idea with the RFC is mostly to revive the discussion to fix the 
difference between the style docs and the code, so I hope it's not 
completely unresolvable

Also my emails to Markus are bouncing back so hopefully he finds this 
patch in the qemu list.

>
> (1) I want checkpatch not to warn about line lengths that in fact
> we'd be happy to include in the tree (because it's noise, and it
> pushes people into wrapping the cases which the style guide describes
> as "awkwardly wrapped" and better not wrapped, to silence the warning),
> so I don't want checkpatch to even warn on less than 90 or 100 chars.
> This is effectively also the position that the Linux kernel
> checkpatch takes these days.
Philippe Mathieu-Daudé talked about keeping some error to avoid overly 
long lines, so that part I added to my patch.
>
> (2) Markus (as I understand his point of view from the 2020 thread)
> wants checkpatch to warn about anything over the "this is definitely
> fine" 80 column mark, so that developers are reminded that
> they might want to make a judgement call there.
>
> The coding-style text reflects my point of view, because I
> wrote it, and therefore personally I'm happy to update checkpatch
> to match it :-)
>
>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> index d900d18048..2c2d7b31ab 100755
>> --- a/scripts/checkpatch.pl
>> +++ b/scripts/checkpatch.pl
>> @@ -1639,12 +1639,12 @@ sub process {
>>                  if ($line =~ /^\+/ &&
>>                      !($line =~ /^\+\s*"[^"]*"\s*(?:\s*|,|\)\s*;)\s*$/) &&
>>                      !($rawline =~ /^[^[:alnum:]]*https?:\S*$/) &&
>> -                   $length > 80)
>> +                   $length > 100)
> Gan Qixin's patch has the advantage of putting the max length into
> a variable rather than continuing to hardcode it.
I can send a v2 with these changes if this is the way we're heading, or 
maybe we could use Gan Qixin's patch (although a more detailed commit 
message might be desired)
>
> thanks
> -- PMM
-- 
Lucas Mateus M. Araujo e Castro
Instituto de Pesquisas ELDORADO 
<https://www.eldorado.org.br/?utm_campaign=assinatura_de_e-mail&utm_medium=email&utm_source=RD+Station>
Departamento Computação Embarcada
Analista de Software Trainee
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>