Re: [RFC PATCH RESEND] scripts/checkpatch.pl: Change line limit warning

[Peter Maydell]

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