netdev - Re: Coding Style: Reverse XMAS tree declarations ?

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:   Fri, 4 Nov 2016 21:06:58 +0100
From:   Lino Sanfilippo <LinoSanfilippo@....de>
To:     Joe Perches <joe@...ches.com>, David Miller <davem@...emloft.net>,
        lsanfil@...vell.com
Cc:     madalin.bucur@....com, akpm@...ux-foundation.org, corbet@....net,
        netdev@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org, oss@...error.net,
        ppc@...dchasers.com, pebolle@...cali.nl,
        joakim.tjernlund@...nsmode.se, Randy Dunlap <rdunlap@...radead.org>
Subject: Re: Coding Style: Reverse XMAS tree declarations ?

On 04.11.2016 18:44, Joe Perches wrote:
> On Fri, 2016-11-04 at 11:07 -0400, David Miller wrote:
>> From: Lino Sanfilippo <lsanfil@...vell.com>
>> > On 04.11.2016 07:53, Joe Perches wrote:
>> >> CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to
>> >> shortest
>> >> #446: FILE: drivers/net/ethernet/ethoc.c:446:
>> >> +                    int size = bd.stat >> 16;
>> >> +                    struct sk_buff *skb;
>> > should not this case be valid? Optically the longer line is already
>> > before the shorter.
>> > I think that the whole point in using this reverse xmas tree ordering
>> > is to have
>> > the code optically tidied up and not to enforce ordering between
>> > variable name lengths.
>> 
>> That's correct.
> 
> And also another reason the whole reverse xmas tree
> automatic declaration layout concept is IMO dubious.
> 
> Basically, you're looking not at the initial ordering
> of automatics as important, but helping find a specific
> automatic when reversing from reading code is not always
> correct.
> 
> Something like:
> 
> static void function{args,...)
> {
> 	[longish list of reverse xmas tree identifiers...]
> 	struct foo *bar = longish_function(args, ...);
> 	struct foobarbaz *qux;
> 	[more identifers]
> 
> 	[multiple screenfuls of code later...)
> 
> 	new_function(..., bar, ...);
> 
> 	[more code...]
> }
> 
> and the reverse xmas tree helpfulness of looking up the
> type of bar is neither obvious nor easy.
> 

In this case it is IMHO rather the declaration + initialization that makes
"bar" hard to find at one glance, not the use of RXT. You could do something like

 	[longish list of reverse xmas tree identifiers...]
 	struct foobarbaz *qux;
 	struct foo *bar;

 	bar = longish_function(args, ...);

to increase readability. 

Personally I find it more readable to always use a separate line for initializations
by means of functions (regardless of whether the RXT scheme is used or not). 
 
> My preference would be for a bar that serves coffee and alcohol.
> 

At least a bar like this should not be too hard to find :)

Regards,
Lino