diff mbox

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

Message ID 1478242438.1924.31.camel@perches.com
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Joe Perches Nov. 4, 2016, 6:53 a.m. UTC 
On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
> From: Madalin Bucur <madalin.bucur@nxp.com>
> 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/

(this likely won't apply because Evolution is a horrible
email client for sending patches, but the attachment should)

An example output:

$ ./scripts/checkpatch.pl -f drivers/net/ethernet/ethoc.c --types=reverse_xmas_tree --show-types
CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#200: FILE: drivers/net/ethernet/ethoc.c:200:
+	void __iomem *iobase;
+	void __iomem *membase;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#202: FILE: drivers/net/ethernet/ethoc.c:202:
+	int dma_alloc;
+	resource_size_t io_region_size;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#305: FILE: drivers/net/ethernet/ethoc.c:305:
+	int i;
+	void *vma;

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;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#515: FILE: drivers/net/ethernet/ethoc.c:515:
+	int count;
+	struct ethoc_bd bd;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#675: FILE: drivers/net/ethernet/ethoc.c:675:
+	struct ethoc *priv = netdev_priv(dev);
+	struct phy_device *phy;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#756: FILE: drivers/net/ethernet/ethoc.c:756:
+	struct ethoc *priv = netdev_priv(dev);
+	struct mii_ioctl_data *mdio = if_mii(ifr);

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#801: FILE: drivers/net/ethernet/ethoc.c:801:
+	u32 mode = ethoc_read(priv, MODER);
+	struct netdev_hw_addr *ha;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#996: FILE: drivers/net/ethernet/ethoc.c:996:
+	struct resource *res = NULL;
+	struct resource *mmio = NULL;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#1001: FILE: drivers/net/ethernet/ethoc.c:1001:
+	int ret = 0;
+	bool random_mac = false;

CHECK:REVERSE_XMAS_TREE: Prefer ordering declarations longest to shortest
#1002: FILE: drivers/net/ethernet/ethoc.c:1002:
+	bool random_mac = false;
+	struct ethoc_platform_data *pdata = dev_get_platdata(&pdev->dev);

total: 0 errors, 0 warnings, 11 checks, 1297 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

drivers/net/ethernet/ethoc.c has style problems, please review.

NOTE: Used message types: REVERSE_XMAS_TREE

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
 scripts/checkpatch.pl | 70 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 14 deletions(-)

Comments

Lino Sanfilippo Nov. 4, 2016, 11:01 a.m. UTC | #1 
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.


Regards,
Lino
David Miller Nov. 4, 2016, 3:07 p.m. UTC | #2 
From: Lino Sanfilippo <lsanfil@marvell.com>
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.
Randy Dunlap Nov. 4, 2016, 5:05 p.m. UTC | #3 
On 11/03/16 23:53, Joe Perches wrote:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur <madalin.bucur@nxp.com>
>> 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.
Joe Perches Nov. 4, 2016, 5:44 p.m. UTC | #4 
On Fri, 2016-11-04 at 11:07 -0400, David Miller wrote:
> From: Lino Sanfilippo <lsanfil@marvell.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.

My preference would be for a bar that serves coffee and alcohol.
David VomLehn Nov. 4, 2016, 7:48 p.m. UTC | #5 
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 <madalin.bucur@nxp.com>
> >> 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 order        Easier 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?
Lino Sanfilippo Nov. 4, 2016, 8:06 p.m. UTC | #6 
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@marvell.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
Michael Ellerman Nov. 7, 2016, 8:05 a.m. UTC | #7 
Joe Perches <joe@perches.com> writes:
> On Thu, 2016-11-03 at 15:58 -0400, David Miller wrote:
>> From: Madalin Bucur <madalin.bucur@nxp.com>
>> 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
David Laight Nov. 7, 2016, 11 a.m. UTC | #8 
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
diff mbox

Patch

 scripts/checkpatch.pl | 70 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 56 insertions(+), 14 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index fdacd759078e..dd344ac77cb8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2114,6 +2114,29 @@  sub pos_last_openparen {
 	return length(expand_tabs(substr($line, 0, $last_openparen))) + 1;
 }
 
+sub get_decl {
+	my ($line) = @_;
+
+	# typical declarations
+	if ($line =~ /^\+\s+($Declare\s*$Ident)\s*[=,;:\[]/) {
+		return $1;
+	}
+	# function pointer declarations
+	if ($line =~ /^\+\s+($Declare\s*\(\s*\*\s*$Ident\s*\))\s*[=,;:\[\(]/) {
+		return $1;
+	}
+	# foo bar; where foo is some local typedef or #define
+	if ($line =~ /^\+\s+($Ident(?:\s+|\s*\*\s*)$Ident)\s*[=,;\[]/) {
+		return $1;
+	}
+	# known declaration macros
+	if ($line =~ /^\+\s+$(declaration_macros)/) {
+		return $1;
+	}
+
+	return undef;
+}
+
 sub process {
 	my $filename = shift;
 
@@ -3063,9 +3086,7 @@  sub process {
 			$last_blank_line = $linenr;
 		}
 
-# check for missing blank lines after declarations
-		if ($sline =~ /^\+\s+\S/ &&			#Not at char 1
-			# actual declarations
+		my $prev_decl =
 		    ($prevline =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
 			# function pointer declarations
 		     $prevline =~ /^\+\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
@@ -3078,25 +3099,30 @@  sub process {
 			# other possible extensions of declaration lines
 		      $prevline =~ /(?:$Compare|$Assignment|$Operators)\s*$/ ||
 			# not starting a section or a macro "\" extended line
-		      $prevline =~ /(?:\{\s*|\\)$/) &&
-			# looks like a declaration
-		    !($sline =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
+		      $prevline =~ /(?:\{\s*|\\)$/);
+		my $sline_decl =
+		    $sline =~ /^\+\s+$Declare\s*$Ident\s*[=,;:\[]/ ||
 			# function pointer declarations
-		      $sline =~ /^\+\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
+		    $sline =~ /^\+\s+$Declare\s*\(\s*\*\s*$Ident\s*\)\s*[=,;:\[\(]/ ||
 			# foo bar; where foo is some local typedef or #define
-		      $sline =~ /^\+\s+$Ident(?:\s+|\s*\*\s*)$Ident\s*[=,;\[]/ ||
+		    $sline =~ /^\+\s+$Ident(?:\s+|\s*\*\s*)$Ident\s*[=,;\[]/ ||
 			# known declaration macros
-		      $sline =~ /^\+\s+$declaration_macros/ ||
+		    $sline =~ /^\+\s+$declaration_macros/ ||
 			# start of struct or union or enum
-		      $sline =~ /^\+\s+(?:union|struct|enum|typedef)\b/ ||
+		    $sline =~ /^\+\s+(?:union|struct|enum|typedef)\b/ ||
 			# start or end of block or continuation of declaration
-		      $sline =~ /^\+\s+(?:$|[\{\}\.\#\"\?\:\(\[])/ ||
+		    $sline =~ /^\+\s+(?:$|[\{\}\.\#\"\?\:\(\[])/ ||
 			# bitfield continuation
-		      $sline =~ /^\+\s+$Ident\s*:\s*\d+\s*[,;]/ ||
+		    $sline =~ /^\+\s+$Ident\s*:\s*\d+\s*[,;]/ ||
 			# other possible extensions of declaration lines
-		      $sline =~ /^\+\s+\(?\s*(?:$Compare|$Assignment|$Operators)/) &&
+		    $sline =~ /^\+\s+\(?\s*(?:$Compare|$Assignment|$Operators)/;
+
+# check for missing blank lines after declarations
+		if ($sline =~ /^\+\s+\S/ &&			#Not at char 1
+			# actual declarations
+		    $prev_decl && !$sline_decl &&
 			# indentation of previous and current line are the same
-		    (($prevline =~ /\+(\s+)\S/) && $sline =~ /^\+$1\S/)) {
+		    ($prevline =~ /\+(\s+)\S/ && $sline =~ /^\+$1\S/)) {
 			if (WARN("LINE_SPACING",
 				 "Missing a blank line after declarations\n" . $hereprev) &&
 			    $fix) {
@@ -3104,6 +3130,22 @@  sub process {
 			}
 		}
 
+# check for reverse christmas tree declarations in net/ and drivers/net/
+		if ($realfile =~ m@^(?:drivers/net/|net/)@ &&
+		    $sline =~ /^\+\s+\S/ &&			#Not at char 1
+			# actual declarations
+		    $prev_decl && $sline_decl &&
+			# indentation of previous and current line are the same
+		    (($prevline =~ /\+(\s+)\S/) && $sline =~ /^\+$1\S/)) {
+			my $p = get_decl($prevline);
+			my $l = get_decl($sline);
+			if (defined($p) && defined($l) && length($p) < length($l) &&
+			    CHK("REVERSE_XMAS_TREE",
+				"Prefer ordering declarations longest to shortest\n" . $hereprev) &&
+			    $fix) {
+			}
+		}
+
 # check for spaces at the beginning of a line.
 # Exceptions:
 #  1) within comments