RE: [PATCH] scripts/checkpatch.pl: Modify the line length limit of the code

[ganqixin]

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