This update brings a large number of small fixes to existing checks.
Of note:
- fixes up a number of false type detections,
- code indent checks handle macros and continuation,
- complex macro detection handles square bracket nesting correctly, and
- better comment handling for case.
Complete changelog below.
*** BLURB HERE ***
Andy Whitcroft (13):
checkpatch: do is not a possible type
checkpatch: labels are not possible types
checkpatch: handle do without braces if we have enough context
checkpatch: macros which define structure members are not complex
checkpatch: accept any sized le/be type
checkpatch: pull out known acceptable typedefs
checkpatch: suspect code indent must stop at #else/#elif
checkpatch: complex macros checks miss square brackets
checkpatch: DEFINE_ macros are real definitions for exports
checkpatch: trailing statements ensure we report the end of the line
checkpatch: suspect indent handle macro continuation
checkpatch: allow for comments either side of a brace on case
checkpatch: version: 0.24
scripts/checkpatch.pl | 110 +++++++++++++++++++++++++++++++++++++-----------
1 files changed, 85 insertions(+), 25 deletions(-)
A do without braces '{' may trigger a false possible type 'do' and then this
may be interpreted as an external definition of foo():
do
foo();
while (bar);
Add do to the type exclusions. Fix up tests so we can check for them.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 24 ++++++++++++++++++------
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 118fe1f..6b21188 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -913,12 +913,22 @@ sub annotate_values {
sub possible {
my ($possible, $line) = @_;
- print "CHECK<$possible> ($line)\n" if ($dbg_possible > 1);
- if ($possible !~ /^(?:$Modifier|$Storage|$Type|DEFINE_\S+)$/ &&
- $possible ne 'goto' && $possible ne 'return' &&
- $possible ne 'case' && $possible ne 'else' &&
- $possible ne 'asm' && $possible ne '__asm__' &&
- $possible !~ /^(typedef|struct|enum)\b/) {
+ print "CHECK<$possible> ($line)\n" if ($dbg_possible > 2);
+ if ($possible !~ /(?:
+ ^(?:
+ $Modifier|
+ $Storage|
+ $Type|
+ DEFINE_\S+|
+ goto|
+ return|
+ case|
+ else|
+ asm|__asm__|
+ do
+ )$|
+ ^(?:typedef|struct|enum)\b
+ )/x) {
# Check for modifiers.
$possible =~ s/\s*$Storage\s*//g;
$possible =~ s/\s*$Sparse\s*//g;
@@ -936,6 +946,8 @@ sub possible {
push(@typeList, $possible);
}
build_types();
+ } else {
+ warn "NOTPOSS: $possible ($line)\n" if ($dbg_possible > 1);
}
}
--
1.6.0.1.451.gc8d31
A label is not a candidate for a possible type. Exclude them.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6b21188..17e1d94 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1324,7 +1324,7 @@ sub process {
possible($type, "A:" . $s);
# definitions in global scope can only start with types
- } elsif ($s =~ /^.(?:$Storage\s+)?(?:$Inline\s+)?(?:const\s+)?($Ident)\b/s) {
+ } elsif ($s =~ /^.(?:$Storage\s+)?(?:$Inline\s+)?(?:const\s+)?($Ident)\b\s*(?!:)/s) {
possible($1, "B:" . $s);
}
--
1.6.0.1.451.gc8d31
We are likely going to have 24 bit types. Expand the type matcher to match
any size.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6eceda7..bb88df2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -159,7 +159,7 @@ our @typeList = (
qr{float},
qr{double},
qr{bool},
- qr{(?:__)?(?:u|s|be|le)(?:8|16|32|64)},
+ qr{(?:__)?(?:u|s|be|le)(?:\d|\d\d)},
qr{struct\s+$Ident},
qr{union\s+$Ident},
qr{enum\s+$Ident},
--
1.6.0.1.451.gc8d31
If we have sufficient context detect and handle do without braces ({).
Else these incorrectly trigger a trailing statements error for the
associated while.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 27 ++++++++++++++++++++++++---
1 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 17e1d94..19690a2 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1051,6 +1051,7 @@ sub process {
# suppression flags
my %suppress_ifbraces;
+ my %suppress_whiletrailers;
# Pre-scan the patch sanitizing the lines.
# Pre-scan the patch looking for any __setup documentation.
@@ -1156,6 +1157,7 @@ sub process {
$prev_values = 'E';
%suppress_ifbraces = ();
+ %suppress_whiletrailers = ();
next;
# track the line number as we move through the hunk, note that
@@ -1301,9 +1303,9 @@ sub process {
}
# Check for potential 'bare' types
- my ($stat, $cond, $line_nr_next, $remain_next);
+ my ($stat, $cond, $line_nr_next, $remain_next, $off_next);
if ($realcnt && $line =~ /.\s*\S/) {
- ($stat, $cond, $line_nr_next, $remain_next) =
+ ($stat, $cond, $line_nr_next, $remain_next, $off_next) =
ctx_statement_block($linenr, $realcnt, 0);
$stat =~ s/\n./\n /g;
$cond =~ s/\n./\n /g;
@@ -1952,7 +1954,26 @@ sub process {
# Check for illegal assignment in if conditional -- and check for trailing
# statements after the conditional.
- if ($line =~ /\b(?:if|while|for)\s*\(/ && $line !~ /^.\s*#/) {
+ if ($line =~ /do\s*(?!{)/) {
+ my ($stat_next) = ctx_statement_block($line_nr_next,
+ $remain_next, $off_next);
+ $stat_next =~ s/\n./\n /g;
+ ##print "stat<$stat> stat_next<$stat_next>\n";
+
+ if ($stat_next =~ /^\s*while\b/) {
+ # If the statement carries leading newlines,
+ # then count those as offsets.
+ my ($whitespace) =
+ ($stat_next =~ /^((?:\s*\n[+-])*\s*)/s);
+ my $offset =
+ statement_rawlines($whitespace) - 1;
+
+ $suppress_whiletrailers{$line_nr_next +
+ $offset} = 1;
+ }
+ }
+ if (!defined $suppress_whiletrailers{$linenr} &&
+ $line =~ /\b(?:if|while|for)\s*\(/ && $line !~ /^.\s*#/) {
my ($s, $c) = ($stat, $cond);
if ($c =~ /\bif\s*\(.*[^<>!=]=[^=].*/) {
--
1.6.0.1.451.gc8d31
We often see macros which define structure members, these are not
complex and necessarily do not have braces or brackets. For example:
#define _PLIST_HEAD_INIT(head) \
.prio_list = LIST_HEAD_INIT((head).prio_list), \
.node_list = LIST_HEAD_INIT((head).node_list)
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 19690a2..6eceda7 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2155,6 +2155,7 @@ sub process {
if ($dstat ne '' &&
$dstat !~ /^(?:$Ident|-?$Constant)$/ &&
$dstat !~ /$exceptions/ &&
+ $dstat !~ /^\.$Ident\s*=/ &&
$dstat =~ /$Operators/)
{
ERROR("Macros with complex values should be enclosed in parenthesis\n" . "$here\n$ctx\n");
--
1.6.0.1.451.gc8d31
When we hit and #else or #elif we know we are meeting an alternative
piece of code. All bets are off on indent if we did not see the open of
the control so stop checking.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 4680ccf..c479bde 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1468,6 +1468,12 @@ sub process {
while ($cond_ptr != $cond_lines) {
$cond_ptr = $cond_lines;
+ # If we see an #else/#elif then the code
+ # is not linear.
+ if ($s =~ /^\s*\#\s*(?:else|elif)/) {
+ $check = 0;
+ }
+
# Ignore:
# 1) blank lines, they should be at 0,
# 2) preprocessor lines, and
--
1.6.0.1.451.gc8d31
Within the type checker we have a number of common kernel types which
must be implemented as typedefs. Pull those out so that we can use
the same expressions to trigger exclusions.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 8 +++++++-
1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bb88df2..4680ccf 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -146,6 +146,11 @@ our $UTF8 = qr {
| \xF4[\x80-\x8F][\x80-\xBF]{2} # plane 16
}x;
+our $typeTypedefs = qr{(?x:
+ (?:__)?(?:u|s|be|le)(?:\d|\d\d)|
+ atomic_t
+)};
+
our @typeList = (
qr{void},
qr{(?:unsigned\s+)?char},
@@ -159,7 +164,6 @@ our @typeList = (
qr{float},
qr{double},
qr{bool},
- qr{(?:__)?(?:u|s|be|le)(?:\d|\d\d)},
qr{struct\s+$Ident},
qr{union\s+$Ident},
qr{enum\s+$Ident},
@@ -179,6 +183,7 @@ sub build_types {
(?:$Modifier\s+|const\s+)*
(?:
(?:typeof|__typeof__)\s*\(\s*\**\s*$Ident\s*\)|
+ (?:$typeTypedefs\b)|
(?:${all}\b)
)
(?:\s+$Modifier|\s+const)*
@@ -1589,6 +1594,7 @@ sub process {
if ($line =~ /\btypedef\s/ &&
$line !~ /\btypedef\s+$Type\s+\(\s*\*?$Ident\s*\)\s*\(/ &&
$line !~ /\btypedef\s+$Type\s+$Ident\s*\(/ &&
+ $line !~ /\b$typeTypedefs\b/ &&
$line !~ /\b__bitwise(?:__|)\b/) {
WARN("do not add new typedefs\n" . $herecurr);
}
--
1.6.0.1.451.gc8d31
We are missing 'simple' values which include square brackets. Refactor
to ensure we handle nesting correctly and detect these simple forms.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index c479bde..54dfa2b 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2142,9 +2142,10 @@ sub process {
$dstat =~ s/\s*$//s;
# Flatten any parentheses and braces
- while ($dstat =~ s/\([^\(\)]*\)/1/) {
- }
- while ($dstat =~ s/\{[^\{\}]*\}/1/) {
+ while ($dstat =~ s/\([^\(\)]*\)/1/ ||
+ $dstat =~ s/\{[^\{\}]*\}/1/ ||
+ $dstat =~ s/\[[^\{\}]*\]/1/)
+ {
}
my $exceptions = qr{
--
1.6.0.1.451.gc8d31
When we want to confirm an export is directly after its definition we need
to allow for DEFINE_ style macros. Add these to the execeptions. Refactor
the exceptions.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 15 ++++++++-------
1 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 54dfa2b..a675f06 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1573,13 +1573,14 @@ sub process {
if (($line =~ /EXPORT_SYMBOL.*\((.*)\)/) ||
($line =~ /EXPORT_UNUSED_SYMBOL.*\((.*)\)/)) {
my $name = $1;
- if (($prevline !~ /^}/) &&
- ($prevline !~ /^\+}/) &&
- ($prevline !~ /^ }/) &&
- ($prevline !~ /^.DECLARE_$Ident\(\Q$name\E\)/) &&
- ($prevline !~ /^.LIST_HEAD\(\Q$name\E\)/) &&
- ($prevline !~ /^.$Type\s*\(\s*\*\s*\Q$name\E\s*\)\s*\(/) &&
- ($prevline !~ /\b\Q$name\E(?:\s+$Attribute)?\s*(?:;|=|\[)/)) {
+ if ($prevline !~ /(?:
+ ^.}|
+ ^.DEFINE_$Ident\(\Q$name\E\)|
+ ^.DECLARE_$Ident\(\Q$name\E\)|
+ ^.LIST_HEAD\(\Q$name\E\)|
+ ^.$Type\s*\(\s*\*\s*\Q$name\E\s*\)\s*\(|
+ \b\Q$name\E(?:\s+$Attribute)?\s*(?:;|=|\[)
+ )/x) {
WARN("EXPORT_SYMBOL(foo); should immediately follow its function/variable\n" . $herecurr);
}
}
--
1.6.0.1.451.gc8d31
When reporting some complex trailing statements we report only the
starting line of the error, that tends to imply the shown line is in
error and confuse the reader. As we do know where the actual error is
report that line too with an appropriate gap marker where applicable.
#ERROR: trailing statements should be on next line
#1: FILE: Z202.c:1:
+ for (pbh = page_buffers(bh->b_page); pbh != bh;
+ pbh = pbh->b_this_page, key++);
#ERROR: trailing statements should be on next line
#4: FILE: Z202.c:4:
+ for (pbh = page_buffers(bh->b_page);
[...]
+ pbh = pbh->b_this_page, key++);
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index a675f06..2c1afba 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2001,7 +2001,16 @@ sub process {
if (length($c) && $s !~ /^\s*{?\s*\\*\s*$/ &&
$c !~ /}\s*while\s*/)
{
- ERROR("trailing statements should be on next line\n" . $herecurr);
+ # Find out how long the conditional actually is.
+ my @newlines = ($c =~ /\n/gs);
+ my $cond_lines = 1 + $#newlines;
+
+ my $stat_real = raw_line($linenr, $cond_lines);
+ if (defined($stat_real) && $cond_lines > 1) {
+ $stat_real = "[...]\n$stat_real";
+ }
+
+ ERROR("trailing statements should be on next line\n" . $herecurr . $stat_real);
}
}
--
1.6.0.1.451.gc8d31
When ignoring a macro in the middle of a conditional, we need to ignore
the macro start and any continuation lines.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 2c1afba..862e8e0 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1465,6 +1465,7 @@ sub process {
}
my $cond_ptr = -1;
+ $continuation = 0;
while ($cond_ptr != $cond_lines) {
$cond_ptr = $cond_lines;
@@ -1478,9 +1479,11 @@ sub process {
# 1) blank lines, they should be at 0,
# 2) preprocessor lines, and
# 3) labels.
- if ($s =~ /^\s*?\n/ ||
+ if ($continuation ||
+ $s =~ /^\s*?\n/ ||
$s =~ /^\s*#\s*?/ ||
$s =~ /^\s*$Ident\s*:/) {
+ $continuation = ($s =~ /^.*?\\\n/) ? 1 : 0;
$s =~ s/^.*?\n//;
$cond_lines++;
}
--
1.6.0.1.451.gc8d31
When specifying case we may have comments and/or braces at the end without
actually having a 'statement'. Allow for these to occur in any order.
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 862e8e0..ada27c4 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -2043,7 +2043,7 @@ sub process {
# case and default should not have general statements after them
if ($line =~ /^.\s*(?:case\s*.*|default\s*):/g &&
$line !~ /\G(?:
- (?:\s*{)?(?:\s*$;*)(?:\s*\\)?\s*$|
+ (?:\s*$;*)(?:\s*{)?(?:\s*$;*)(?:\s*\\)?\s*$|
\s*return\s+
)/xg)
{
--
1.6.0.1.451.gc8d31
Signed-off-by: Andy Whitcroft <[email protected]>
---
scripts/checkpatch.pl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ada27c4..e30bac1 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -9,7 +9,7 @@ use strict;
my $P = $0;
$P =~ s@.*/@@g;
-my $V = '0.23';
+my $V = '0.24';
use Getopt::Long qw(:config no_auto_abbrev);
--
1.6.0.1.451.gc8d31