RE: Coding Style: Reverse XMAS tree declarations ?

[David Laight]

From: Lino Sanfilippo
> Sent: 04 November 2016 20:07
...
> 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).

I find it best to only use initialisers for 'variables' that are (mostly) 
constant.
If something need to be set to NULL in case a search fails, set it to NULL
just before the loop.
Don't put initialisation on the declaration 'because you can'.

Difficulty in spotting the type of a variable is why (IMHO) you should
but all declarations at the top of a function
(except, maybe, temporaries needed for a few lines).

David

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Michael Ellerman]

Joe Perches  writes:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur 
>> Date: Wed, 2 Nov 2016 22:17:26 +0200
>> 
>> > This introduces the Freescale Data Path Acceleration Architecture
>> > +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
>> > +{
>> > + u8 i;
>> > + size_t res = DPAA_BP_RAW_SIZE / 2;
>> 
>> Always order local variable declarations from longest to shortest line,
>> also know as Reverse Christmas Tree Format.
>
> I think this declaration sorting order is misguided but
> here's a possible change to checkpatch adding a test for it
> that does this test just for net/ and drivers/net/

And arch/powerpc too please.

cheers

Re: Coding Style: Reverse XMAS tree declarations ?

[Lino Sanfilippo]

On 04.11.2016 18:44, Joe Perches wrote:
> On Fri, 2016-11-04 at 11:07 -0400, David Miller wrote:
>> From: Lino Sanfilippo 
>> > 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

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[David VomLehn]

On Fri, Nov 04, 2016 at 10:05:15AM -0700, Randy Dunlap wrote:
> On 11/03/16 23:53, Joe Perches wrote:
> > On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
> >> From: Madalin Bucur 
> >> Date: Wed, 2 Nov 2016 22:17:26 +0200
> >>
> >>> This introduces the Freescale Data Path Acceleration Architecture
> >>> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
> >>> +{
> >>> + u8 i;
> >>> + size_t res = DPAA_BP_RAW_SIZE / 2;
> >>
> >> Always order local variable declarations from longest to shortest line,
> >> also know as Reverse Christmas Tree Format.
> > 
> > I think this declaration sorting order is misguided but
> > here's a possible change to checkpatch adding a test for it
> > that does this test just for net/ and drivers/net/
> 
> I agree with the misguided part.
> That's not actually in CodingStyle AFAICT. Where did this come from?
> 
> 
> thanks.
> -- 
> ~Randy

This puzzles me. The CodingStyle gives some pretty reasonable rationales
for coding style over above the "it's easier to read if it all looks the
same". I can see rationales for other approaches (and I am not proposing
any of these):

alphabetic orderEasier to search for declarations
complex to simple   As in, structs and unions, pointers to simple
data (int, char), simple data. It seems like I
can deduce the simple types from usage, but more
complex I need to know things like the
particular structure.
group by usage  Mirror the ontological locality in the code

Do we have a basis for thinking this is easier or more consistent than
any other approach?
-- 
David VL

Re: Coding Style: Reverse XMAS tree declarations ?

[Joe Perches]

On Fri, 2016-11-04 at 11:07 -0400, David Miller wrote:
> From: Lino Sanfilippo 
> > 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.

My preference would be for a bar that serves coffee and alcohol.

Re: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)

[Randy Dunlap]

On 11/03/16 23:53, Joe Perches wrote:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur 
>> Date: Wed, 2 Nov 2016 22:17:26 +0200
>>
>>> This introduces the Freescale Data Path Acceleration Architecture
>>> +static inline size_t bpool_buffer_raw_size(u8 index, u8 cnt)
>>> +{
>>> + u8 i;
>>> + size_t res = DPAA_BP_RAW_SIZE / 2;
>>
>> Always order local variable declarations from longest to shortest line,
>> also know as Reverse Christmas Tree Format.
> 
> I think this declaration sorting order is misguided but
> here's a possible change to checkpatch adding a test for it
> that does this test just for net/ and drivers/net/

I agree with the misguided part.
That's not actually in CodingStyle AFAICT. Where did this come from?


thanks.
-- 
~Randy

Re: Coding Style: Reverse XMAS tree declarations ?

[David Miller]

From: Lino Sanfilippo 
Date: Fri, 4 Nov 2016 12:01:17 +0100

> Hi,
> 
> 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.