| Message ID | 20220606143419.656598-1-lucas.araujo@eldorado.org.br (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,RESEND] scripts/checkpatch.pl: Change line limit warning | expand |
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
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"); }
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(-)