linux-kernel - Re: [PATCH] Drop 80-character limit in checkpatch.pl

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] [day] [month] [year] [list]

Date:	Sun, 27 Dec 2009 12:15:00 -0500
From:	Jon Smirl <jonsmirl@...il.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc:	Jiri Kosina <jkosina@...e.cz>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Américo Wang <xiyou.wangcong@...il.com>,
	Mikulas Patocka <mpatocka@...hat.com>,
	linux-kernel@...r.kernel.org, Alasdair G Kergon <agk@...hat.com>,
	dm-devel@...hat.com
Subject: Re: [PATCH] Drop 80-character limit in checkpatch.pl

On Fri, Dec 18, 2009 at 8:55 AM, Bartlomiej Zolnierkiewicz
<bzolnier@...il.com> wrote:
> On Friday 18 December 2009 02:04:37 pm Jiri Kosina wrote:
>> On Thu, 17 Dec 2009, Bartlomiej Zolnierkiewicz wrote:
>>
>> > > > I like this patch, this is actually what I wanted to do.
>> > >
>> > > I have nothing against a switch, but it had better default to off.
>> > >
>> > > The whole 80-char limit is insane. It results in insane "fixes". Just
>> > > about every time somebody "improves" a patch due to the warning, the
>> > > result is worse than the original patch.
>> >
>> > Examples? :)
>>
>> balance_leaf() in fs/reiserfs/do_balan.c
>>
>> Example picked totally at random:
>>
>>       set_le_ih_k_offset(ih,
>>                          le_ih_k_offset(ih) +
>>                          (tb->
>>                           lbytes <<
>>                           (is_indirect_le_ih
>>                            (ih) ? tb->tb_sb->
>>                            s_blocksize_bits -
>>                            UNFM_P_SHIFT :
>>                            0)));
>>
>> See how everything is nicely aligned to 80 cols?
>
> I see but the above code is an utter crap anyway.
>
> Firstly what kind of a function parameter is that:
>
>   le_ih_k_offset(ih) + (tb->lbytes << (is_indirect_le_ih(ih) ? tb->tb_sb->s_blocksize_bits - UNFM_P_SHIFT : 0))
>
> ?
>
> [ BTW 'tb->tb_sb->s_blocksize_bits - UNFM_P_SHIFT' construct is used five
>  times in balance_leaf() and is a likely candidate for helper / macro. ]
>
> More importantly the whole balance_leaf() function is almost 1400 LOC (!)
> big and impossible to read: code for handling particular 'switch' blocks
> should be factored out into separate functions etc.
>
> The point I was making is that the once we remove the limit we don't have
> other tool to _automatically_ point suspicious code areas (yes, I would
> also prefer intelligent static code checker over dumb limit but it simply

A static code checker (like sparse) could do much more intelligent
wrapping analysis.  Maybe someone will take this up as a thesis
project and provide a replacement for indent. Post example
reformattings to LKML until the formatting gods are satisfied.

The problem I see is that C statements that over over 50 chars or so
should be flagged if they aren't declarations or strings. That's an
independent problem from excessive nesting. Tools like ident aren't
smart enough to separate these problems and flags the combination of
statement length and nesting.

I'd be a lot happier if someone simply patched out all of the trailing
whitespace that has gotten in to the kernel tree over the years.
jonsmirl@...ra:~/fs$ grep -rI [[:space:]]$ * | wc
  47804  231898 2888009

> not here as things are today and dumb limit works surprisingly well most
> of the time -- please note how that the structural problem with the code
> example given is immediately visible with the current limit).
>
>> Generally, don't look at this function after having a good lunch you want
>> to keep. You have been warned.
>
> No worries, I visit dark places (ide, staging, ..) and come back alive.. ;)
>
> --
> Bartlomiej Zolnierkiewicz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>



-- 
Jon Smirl
jonsmirl@...il.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/