From: Joe Perches <joe@perches.com>
To: David Miller <davem@davemloft.net>,
madalin.bucur@nxp.com, Andrew Morton <akpm@linux-foundation.org>,
Jonathan Corbet <corbet@lwn.net>
Cc: netdev@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, oss@buserror.net,
ppc@mindchasers.com, pebolle@tiscali.nl,
joakim.tjernlund@transmode.se
Subject: Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet)
Date: Thu, 03 Nov 2016 23:53:58 -0700 [thread overview]
Message-ID: <1478242438.1924.31.camel@perches.com> (raw)
In-Reply-To: <20161103.155816.642712588084106823.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 7497 bytes --]
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(-)
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
[-- Attachment #2: cp_xmas.diff --]
[-- Type: text/x-patch, Size: 4214 bytes --]
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
next prev parent reply other threads:[~2016-11-04 6:54 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-02 20:17 [PATCH net-next v6 00/10] dpaa_eth: Add the QorIQ DPAA Ethernet driver Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 01/10] devres: add devm_alloc_percpu() Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet Madalin Bucur
2016-11-03 19:58 ` David Miller
2016-11-04 6:53 ` Joe Perches [this message]
2016-11-04 11:01 ` Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet) Lino Sanfilippo
2016-11-04 15:07 ` Coding Style: Reverse XMAS tree declarations ? David Miller
2016-11-04 17:44 ` Joe Perches
2016-11-04 20:06 ` Lino Sanfilippo
2016-11-07 11:00 ` David Laight
2016-11-04 17:05 ` Coding Style: Reverse XMAS tree declarations ? (was Re: [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet) Randy Dunlap
2016-11-04 19:48 ` David VomLehn
2016-11-07 8:05 ` Michael Ellerman
2016-11-07 15:43 ` [PATCH net-next v6 02/10] dpaa_eth: add support for DPAA Ethernet Madalin-Cristian Bucur
2016-11-07 15:55 ` David Miller
2016-11-07 16:32 ` Madalin-Cristian Bucur
2016-11-07 16:39 ` David Miller
2016-11-07 16:59 ` Madalin-Cristian Bucur
2016-11-09 17:16 ` Madalin-Cristian Bucur
2016-11-09 17:18 ` David Miller
2016-11-07 16:25 ` Joakim Tjernlund
2016-11-02 20:17 ` [PATCH net-next v6 03/10] dpaa_eth: add option to use one buffer pool set Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 04/10] dpaa_eth: add ethtool functionality Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 05/10] dpaa_eth: add ethtool statistics Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 06/10] dpaa_eth: add sysfs exports Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 07/10] dpaa_eth: add trace points Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 08/10] arch/powerpc: Enable FSL_PAMU Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 09/10] arch/powerpc: Enable FSL_FMAN Madalin Bucur
2016-11-02 20:17 ` [PATCH net-next v6 10/10] arch/powerpc: Enable dpaa_eth Madalin Bucur
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1478242438.1924.31.camel@perches.com \
--to=joe@perches.com \
--cc=akpm@linux-foundation.org \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=joakim.tjernlund@transmode.se \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=madalin.bucur@nxp.com \
--cc=netdev@vger.kernel.org \
--cc=oss@buserror.net \
--cc=pebolle@tiscali.nl \
--cc=ppc@mindchasers.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).