linux-kernel - Re: [Ksummit-discuss] checkkpatch (in)sanity ?

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 29 Aug 2016 12:08:16 -0700
From:   Josh Triplett <josh@...htriplett.org>
To:     Joe Perches <joe@...ches.com>
Cc:     "Luck, Tony" <tony.luck@...el.com>,
        "Levin, Alexander" <alexander.levin@...izon.com>,
        Greg KH <gregkh@...uxfoundation.org>,
        Sasha Levin <levinsasha928@...il.com>,
        "ksummit-discuss@...ts.linuxfoundation.org" 
        <ksummit-discuss@...ts.linuxfoundation.org>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [Ksummit-discuss] checkkpatch (in)sanity ?

On Mon, Aug 29, 2016 at 11:01:40AM -0700, Joe Perches wrote:
> On Mon, 2016-08-29 at 17:46 +0000, Luck, Tony wrote:
> > > 
> > > 80 columns is simply silly when dealing with either
> > > long identifiers or many levels of indentation.
> > > 
> > > One thing that 80 column limit does do is encourage
> > > shorter identifiers and fewer levels of indentation.
> > > 
> > > Generally, both of those are good things.
> > I think the main complaint with the limit is that people fix it by simply
> > breaking the long line, which often makes for less readable code.
> > 
> > Perhaps there would be less pushback on this if checkpatch also
> > complained about clumsily broken long lines and offered the advice
> > to restructure the code with helper functions etc. to avoid deep
> > indentation?
> 
> It suggests that already for 6+ leading tabs, but some more
> intelligence for nominally ugly added line breaks would
> definitely help.
> 
> Using longish simple identifiers or multiple dereferences
> can make the line breaks at 80 columns silly.
> 
> Simple things like:
> 
> 			if (longish_identifier != AN_EVEN_LONGER_DEFINED_CONSTANT_VALUE)
> and
> 			if (some_pointer->member[index].another_member >> shift_constant)
> 
> shouldn't really ever be broken into multiple lines,

Agreed.

Honestly, I almost never see a line that should break solely based on
length.  Almost any line that makes sense to break at a given point
would make sense to break at that point even with a target line length
of 200.

For instance:

			if (an_interesting_function(x) == TARGET_VALUE_FOR_X
			    || an_interesting_function(y) == TARGET_VALUE_FOR_Y) {

That line break makes sense whether you want to break lines at 80
characters, 100, or 800.  (You could argue about
before-or-after-operator, or about line alignment.)  In almost no
circumstances would you want to also break around the '==', even though
that second line takes up 82 characters.