* [bug report] checkpatch: if statement does not need to be enclosed in parentheses
@ 2017-04-08 16:07 Alfonso Lima
2017-04-08 17:33 ` Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Alfonso Lima @ 2017-04-08 16:07 UTC (permalink / raw)
To: apw, joe; +Cc: linux-kernel
Hi,
In current linux-next, if you run the script on this piece of
code:
#define FOO(a) \
if (a) { \
something(); \
something_else(); \
}
You should get:
ERROR: Macros with complex values should be enclosed in parentheses
#1: FILE: foo.c:1:
+#define FOO(a) \
+ if (a) { \
+ something(); \
+ something_else(); \
+ }
We could silence checkpatch.pl using "do {} while ()" around the
if statement. However, the "if () {}" statement should be
enough. If someone could confirm this, I'll go and fix it.
Thanks,
Alfonso
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] checkpatch: if statement does not need to be enclosed in parentheses
2017-04-08 16:07 [bug report] checkpatch: if statement does not need to be enclosed in parentheses Alfonso Lima
@ 2017-04-08 17:33 ` Joe Perches
2017-04-09 8:55 ` Andreas Mohr
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2017-04-08 17:33 UTC (permalink / raw)
To: Alfonso Lima, apw; +Cc: linux-kernel
On Sat, 2017-04-08 at 17:07 +0100, Alfonso Lima wrote:
> Hi,
>
> In current linux-next, if you run the script on this piece of
> code:
>
> #define FOO(a) \
> if (a) { \
> something(); \
> something_else(); \
> }
>
> You should get:
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #1: FILE: foo.c:1:
> +#define FOO(a) \
> + if (a) { \
> + something(); \
> + something_else(); \
> + }
>
> We could silence checkpatch.pl using "do {} while ()" around the
> if statement. However, the "if () {}" statement should be
> enough. If someone could confirm this, I'll go and fix it.
Multiple if/else use is the reason do {} while (0) is suggested.
if (bar())
FOO(a);
else
baz(b);
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [bug report] checkpatch: if statement does not need to be enclosed in parentheses
2017-04-08 17:33 ` Joe Perches
@ 2017-04-09 8:55 ` Andreas Mohr
2017-04-09 17:45 ` [PATCH 1/2] checkpatch: Clarify the EMBEDDED_FUNCTION_NAME message Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Andreas Mohr @ 2017-04-09 8:55 UTC (permalink / raw)
To: Joe Perches; +Cc: Alfonso Lima, apw, linux-kernel
On Sat, Apr 08, 2017 at 10:33:19AM -0700, Joe Perches wrote:
> On Sat, 2017-04-08 at 17:07 +0100, Alfonso Lima wrote:
> > Hi,
> >
> > In current linux-next, if you run the script on this piece of
> > code:
> >
> > #define FOO(a) \
> > if (a) { \
> > something(); \
> > something_else(); \
> > }
> >
> > You should get:
> >
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #1: FILE: foo.c:1:
> > +#define FOO(a) \
> > + if (a) { \
> > + something(); \
> > + something_else(); \
> > + }
> >
> > We could silence checkpatch.pl using "do {} while ()" around the
> > if statement. However, the "if () {}" statement should be
> > enough. If someone could confirm this, I'll go and fix it.
>
> Multiple if/else use is the reason do {} while (0) is suggested.
>
> if (bar())
> FOO(a);
> else
> baz(b);
>
So, if I read this right checkpatch.pl is generating a correct complaint
in this case after all?
However, then we seem to have the following issues remaining:
- "Macros with complex values should be enclosed in parentheses" != "do {} while (0) is suggested" --> this is an imprecision which may cause confusion
- *further* cases of this issue occurring may just as well suggest again
"fixing" (i.e., "damaging") checkpatch.pl rather than
simply obeying (following through on) its correct diagnosis
--> checkpatch.pl behaviour is confusing enough that
it may prompt incorrect core handling "repair actions",
and this danger will remain permanently until
checkpatch.pl reporting is improved to be
less confusing/insufficient about its diagnosis
Or perhaps this imprecision here
again simply is due to
the very frequently observable case of
saying "thou shan't do this" without then also
giving some tried-and-true *reason(s)* for such advice.
("ERROR: Macros with complex values should be enclosed in parentheses")
HTH,
Andreas Mohr
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/2] checkpatch: Clarify the EMBEDDED_FUNCTION_NAME message
2017-04-09 8:55 ` Andreas Mohr
@ 2017-04-09 17:45 ` Joe Perches
2017-04-09 17:45 ` [PATCH 2/2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test Joe Perches
0 siblings, 1 reply; 8+ messages in thread
From: Joe Perches @ 2017-04-09 17:45 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: linux-kernel
Try to make the conversion of embedded function names to "%s: ", __func__
a bit clearer.
Add a bit more information to the comment describing the test too.
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 089c974aa3a5..e8d8481b24c8 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5172,14 +5172,16 @@ sub process {
"break quoted strings at a space character\n" . $hereprev);
}
-#check for an embedded function name in a string when the function is known
-# as part of a diff. This does not work for -f --file checking as it
-#depends on patch context providing the function name
+# check for an embedded function name in a string when the function is known
+# This does not work very well for -f --file checking as it depends on patch
+# context providing the function name or a single line form for in-file
+# function declarations
if ($line =~ /^\+.*$String/ &&
defined($context_function) &&
- get_quoted_string($line, $rawline) =~ /\b$context_function\b/) {
+ get_quoted_string($line, $rawline) =~ /\b$context_function\b/ &&
+ length(get_quoted_string($line, $rawline)) != (length($context_function) + 2)) {
WARN("EMBEDDED_FUNCTION_NAME",
- "Prefer using \"%s\", __func__ to embedded function names\n" . $herecurr);
+ "Prefer using '\"%s...\", __func__' to using '$context_function', this function's name, in a string\n" . $herecurr);
}
# check for spaces before a quoted newline
--
2.10.0.rc2.1.g053435c
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test
2017-04-09 17:45 ` [PATCH 1/2] checkpatch: Clarify the EMBEDDED_FUNCTION_NAME message Joe Perches
@ 2017-04-09 17:45 ` Joe Perches
2017-04-09 17:46 ` Joe Perches
2017-04-09 17:59 ` [PATCH V2] " Joe Perches
0 siblings, 2 replies; 8+ messages in thread
From: Joe Perches @ 2017-04-09 17:45 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: Alfonso Lima, Andreas Mohr, linux-kernel
The logic currrently misses macros that start with an if statement.
e.g.: #define foo(bar) if (bar) baz;
Add a test for macro content that starts with if
Original-patch-by: Alfonso Lima <alfonsolimaastor@gmail.com>
Reported-by: Andreas Mohr <andi@lisas.de>
Signed-off-by: Joe Perches <joe@perches.com>
---
scripts/checkpatch.pl | 746 ++++++++++++--------------------------------------
1 file changed, 170 insertions(+), 576 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e8d8481b24c8..85ec17018cef 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -27,15 +27,12 @@ my $emacs = 0;
my $terse = 0;
my $showfile = 0;
my $file = 0;
-my $git = 0;
-my %git_commits = ();
my $check = 0;
my $check_orig = 0;
my $summary = 1;
my $mailback = 0;
my $summary_file = 0;
my $show_types = 0;
-my $list_types = 0;
my $fix = 0;
my $fix_inplace = 0;
my $root;
@@ -54,9 +51,7 @@ my $min_conf_desc_length = 4;
my $spelling_file = "$D/spelling.txt";
my $codespell = 0;
my $codespellfile = "/usr/share/codespell/dictionary.txt";
-my $conststructsfile = "$D/const_structs.checkpatch";
my $color = 1;
-my $allow_c99_comments = 1;
sub help {
my ($exitcode) = @_;
@@ -73,24 +68,13 @@ Options:
--emacs emacs compile window format
--terse one line per report
--showfile emit diffed file position, not input file position
- -g, --git treat FILE as a single commit or git revision range
- single git commit with:
- <rev>
- <rev>^
- <rev>~n
- multiple git commits with:
- <rev1>..<rev2>
- <rev1>...<rev2>
- <rev>-<count>
- git merges are ignored
-f, --file treat FILE as regular source file
--subjective, --strict enable more subjective tests
- --list-types list the possible message types
--types TYPE(,TYPE2...) show only these comma separated message types
--ignore TYPE(,TYPE2...) ignore various comma separated message types
- --show-types show the specific message type in the output
--max-line-length=n set the maximum line length, if exceeded, warn
--min-conf-desc-length=n set the min description length, if shorter, warn
+ --show-types show the message "types" in the output
--root=PATH PATH to the kernel tree root
--no-summary suppress the per-file summary
--mailback only produce a report in case of warnings/errors
@@ -122,37 +106,6 @@ EOM
exit($exitcode);
}
-sub uniq {
- my %seen;
- return grep { !$seen{$_}++ } @_;
-}
-
-sub list_types {
- my ($exitcode) = @_;
-
- my $count = 0;
-
- local $/ = undef;
-
- open(my $script, '<', abs_path($P)) or
- die "$P: Can't read '$P' $!\n";
-
- my $text = <$script>;
- close($script);
-
- my @types = ();
- for ($text =~ /\b(?:(?:CHK|WARN|ERROR)\s*\(\s*"([^"]+)")/g) {
- push (@types, $_);
- }
- @types = sort(uniq(@types));
- print("#\tMessage type\n\n");
- foreach my $type (@types) {
- print(++$count . "\t" . $type . "\n");
- }
-
- exit($exitcode);
-}
-
my $conf = which_conf($configuration_file);
if (-f $conf) {
my @conf_args;
@@ -188,13 +141,11 @@ GetOptions(
'terse!' => \$terse,
'showfile!' => \$showfile,
'f|file!' => \$file,
- 'g|git!' => \$git,
'subjective!' => \$check,
'strict!' => \$check,
'ignore=s' => \@ignore,
'types=s' => \@use,
'show-types!' => \$show_types,
- 'list-types!' => \$list_types,
'max-line-length=i' => \$max_line_length,
'min-conf-desc-length=i' => \$min_conf_desc_length,
'root=s' => \$root,
@@ -215,8 +166,6 @@ GetOptions(
help(0) if ($help);
-list_types(0) if ($list_types);
-
$fix = 1 if ($fix_inplace);
$check_orig = $check;
@@ -229,9 +178,9 @@ if ($^V && $^V lt $minimum_perl_version) {
}
}
-#if no filenames are given, push '-' to read patch from stdin
if ($#ARGV < 0) {
- push(@ARGV, '-');
+ print "$P: no input files\n";
+ exit(1);
}
sub hash_save_array_words {
@@ -315,12 +264,12 @@ our $Sparse = qr{
__kernel|
__force|
__iomem|
+ __pmem|
__must_check|
__init_refok|
__kprobes|
__ref|
- __rcu|
- __private
+ __rcu
}x;
our $InitAttributePrefix = qr{__(?:mem|cpu|dev|net_|)};
our $InitAttributeData = qr{$InitAttributePrefix(?:initdata\b)};
@@ -335,7 +284,7 @@ our $Attribute = qr{
__percpu|
__nocast|
__safe|
- __bitwise|
+ __bitwise__|
__packed__|
__packed2__|
__naked|
@@ -424,7 +373,7 @@ our $typeTypedefs = qr{(?x:
our $zero_initializer = qr{(?:(?:0[xX])?0+$Int_type?|NULL|false)\b};
our $logFunctions = qr{(?x:
- printk(?:_ratelimited|_once|_deferred_once|_deferred|)|
+ printk(?:_ratelimited|_once|)|
(?:[a-z0-9]+_){1,2}(?:printk|emerg|alert|crit|err|warning|warn|notice|info|debug|dbg|vdbg|devel|cont|WARN)(?:_ratelimited|_once|)|
WARN(?:_RATELIMIT|_ONCE|)|
panic|
@@ -524,11 +473,7 @@ our @mode_permission_funcs = (
["module_param_array_named", 5],
["debugfs_create_(?:file|u8|u16|u32|u64|x8|x16|x32|x64|size_t|atomic_t|bool|blob|regset32|u32_array)", 2],
["proc_create(?:_data|)", 2],
- ["(?:CLASS|DEVICE|SENSOR|SENSOR_DEVICE|IIO_DEVICE)_ATTR", 2],
- ["IIO_DEV_ATTR_[A-Z_]+", 1],
- ["SENSOR_(?:DEVICE_|)ATTR_2", 2],
- ["SENSOR_TEMPLATE(?:_2|)", 3],
- ["__ATTR", 2],
+ ["(?:CLASS|DEVICE|SENSOR)_ATTR", 2],
);
#Create a search pattern for all these functions to speed up a loop below
@@ -546,32 +491,6 @@ our $mode_perms_world_writable = qr{
0[0-7][0-7][2367]
}x;
-our %mode_permission_string_types = (
- "S_IRWXU" => 0700,
- "S_IRUSR" => 0400,
- "S_IWUSR" => 0200,
- "S_IXUSR" => 0100,
- "S_IRWXG" => 0070,
- "S_IRGRP" => 0040,
- "S_IWGRP" => 0020,
- "S_IXGRP" => 0010,
- "S_IRWXO" => 0007,
- "S_IROTH" => 0004,
- "S_IWOTH" => 0002,
- "S_IXOTH" => 0001,
- "S_IRWXUGO" => 0777,
- "S_IRUGO" => 0444,
- "S_IWUGO" => 0222,
- "S_IXUGO" => 0111,
-);
-
-#Create a search pattern for all these strings to speed up a loop below
-our $mode_perms_string_search = "";
-foreach my $entry (keys %mode_permission_string_types) {
- $mode_perms_string_search .= '|' if ($mode_perms_string_search ne "");
- $mode_perms_string_search .= $entry;
-}
-
our $allowed_asm_includes = qr{(?x:
irq|
memory|
@@ -629,29 +548,6 @@ if ($codespell) {
$misspellings = join("|", sort keys %spelling_fix) if keys %spelling_fix;
-my $const_structs = "";
-if (open(my $conststructs, '<', $conststructsfile)) {
- while (<$conststructs>) {
- my $line = $_;
-
- $line =~ s/\s*\n?$//g;
- $line =~ s/^\s*//g;
-
- next if ($line =~ m/^\s*#/);
- next if ($line =~ m/^\s*$/);
- if ($line =~ /\s/) {
- print("$conststructsfile: '$line' invalid - ignored\n");
- next;
- }
-
- $const_structs .= '|' if ($const_structs ne "");
- $const_structs .= $line;
- }
- close($conststructsfile);
-} else {
- warn "No structs that should be const will be found - file '$conststructsfile': $!\n";
-}
-
sub build_types {
my $mods = "(?x: \n" . join("|\n ", (@modifierList, @modifierListFile)) . "\n)";
my $all = "(?x: \n" . join("|\n ", (@typeList, @typeListFile)) . "\n)";
@@ -758,16 +654,6 @@ sub seed_camelcase_file {
}
}
-sub is_maintained_obsolete {
- my ($filename) = @_;
-
- return 0 if (!$tree || !(-e "$root/scripts/get_maintainer.pl"));
-
- my $status = `perl $root/scripts/get_maintainer.pl --status --nom --nol --nogit --nogit-fallback -f $filename 2>&1`;
-
- return $status =~ /obsolete/i;
-}
-
my $camelcase_seeded = 0;
sub seed_camelcase_includes {
return if ($camelcase_seeded);
@@ -865,42 +751,10 @@ my @fixed_inserted = ();
my @fixed_deleted = ();
my $fixlinenr = -1;
-# If input is git commits, extract all commits from the commit expressions.
-# For example, HEAD-3 means we need check 'HEAD, HEAD~1, HEAD~2'.
-die "$P: No git repository found\n" if ($git && !-e ".git");
-
-if ($git) {
- my @commits = ();
- foreach my $commit_expr (@ARGV) {
- my $git_range;
- if ($commit_expr =~ m/^(.*)-(\d+)$/) {
- $git_range = "-$2 $1";
- } elsif ($commit_expr =~ m/\.\./) {
- $git_range = "$commit_expr";
- } else {
- $git_range = "-1 $commit_expr";
- }
- my $lines = `git log --no-color --no-merges --pretty=format:'%H %s' $git_range`;
- foreach my $line (split(/\n/, $lines)) {
- $line =~ /^([0-9a-fA-F]{40,40}) (.*)$/;
- next if (!defined($1) || !defined($2));
- my $sha1 = $1;
- my $subject = $2;
- unshift(@commits, $sha1);
- $git_commits{$sha1} = $subject;
- }
- }
- die "$P: no git commits after extraction!\n" if (@commits == 0);
- @ARGV = @commits;
-}
-
my $vname;
for my $filename (@ARGV) {
my $FILE;
- if ($git) {
- open($FILE, '-|', "git format-patch -M --stdout -1 $filename") ||
- die "$P: $filename: git format-patch failed - $!\n";
- } elsif ($file) {
+ if ($file) {
open($FILE, '-|', "diff -u /dev/null $filename") ||
die "$P: $filename: diff failed - $!\n";
} elsif ($filename eq '-') {
@@ -911,8 +765,6 @@ for my $filename (@ARGV) {
}
if ($filename eq '-') {
$vname = 'Your patch';
- } elsif ($git) {
- $vname = "Commit " . substr($filename, 0, 12) . ' ("' . $git_commits{$filename} . '")';
} else {
$vname = $filename;
}
@@ -1209,11 +1061,6 @@ sub sanitise_line {
$res =~ s@(\#\s*(?:error|warning)\s+).*@$1$clean@;
}
- if ($allow_c99_comments && $res =~ m@(//.*$)@) {
- my $match = $1;
- $res =~ s/\Q$match\E/"$;" x length($match)/e;
- }
-
return $res;
}
@@ -1848,8 +1695,6 @@ my $prefix = '';
sub show_type {
my ($type) = @_;
- $type =~ tr/[a-z]/[A-Z]/;
-
return defined $use_type{$type} if (scalar keys %use_type > 0);
return !defined $ignore_type{$type};
@@ -2135,8 +1980,7 @@ sub process {
my $is_patch = 0;
my $in_header_lines = $file ? 0 : 1;
my $in_commit_log = 0; #Scanning lines before patch
- my $has_commit_log = 0; #Encountered lines before patch
- my $commit_log_possible_stack_dump = 0;
+ my $commit_log_possible_stack_dump = 0;
my $commit_log_long_line = 0;
my $commit_log_has_diff = 0;
my $reported_maintainer_file = 0;
@@ -2156,7 +2000,6 @@ sub process {
my $realline = 0;
my $realcnt = 0;
my $here = '';
- my $context_function; #undef'd unless there's a known function
my $in_comment = 0;
my $comment_edge = 0;
my $first_line = 0;
@@ -2190,13 +2033,12 @@ sub process {
if ($rawline=~/^\+\+\+\s+(\S+)/) {
$setup_docs = 0;
- if ($1 =~ m@Documentation/admin-guide/kernel-parameters.rst$@) {
+ if ($1 =~ m@Documentation/kernel-parameters.txt$@) {
$setup_docs = 1;
}
#next;
}
- if ($rawline=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@(.*)/) {
- my $context = $4;
+ if ($rawline=~/^\@\@ -\d+(?:,\d+)? \+(\d+)(,(\d+))? \@\@/) {
$realline=$1-1;
if (defined $2) {
$realcnt=$3+1;
@@ -2205,12 +2047,6 @@ sub process {
}
$in_comment = 0;
- if ($context =~ /\b(\w+)\s*\(/) {
- $context_function = $1;
- } else {
- undef $context_function;
- }
-
# Guestimate if this is a continuing comment. Run
# the context looking for a comment "edge". If this
# edge is a close comment then we must be in a comment
@@ -2363,10 +2199,6 @@ sub process {
}
if ($found_file) {
- if (is_maintained_obsolete($realfile)) {
- WARN("OBSOLETE",
- "$realfile is marked as 'obsolete' in the MAINTAINERS hierarchy. No unnecessary modifications please.\n");
- }
if ($realfile =~ m@^(?:drivers/net/|net/|drivers/staging/)@) {
$check = 1;
} else {
@@ -2538,9 +2370,8 @@ sub process {
# Check for git id commit length and improperly formed commit descriptions
if ($in_commit_log && !$commit_log_possible_stack_dump &&
- $line !~ /^\s*(?:Link|Patchwork|http|https|BugLink):/i &&
($line =~ /\bcommit\s+[0-9a-f]{5,}\b/i ||
- ($line =~ /(?:\s|^)[0-9a-f]{12,40}(?:[\s"'\(\[]|$)/i &&
+ ($line =~ /\b[0-9a-f]{12,40}\b/i &&
$line !~ /[\<\[][0-9a-f]{12,40}[\>\]]/i &&
$line !~ /\bfixes:\s*[0-9a-f]{12,40}/i))) {
my $init_char = "c";
@@ -2599,7 +2430,6 @@ sub process {
$line =~ /^rename (?:from|to) [\w\/\.\-]+\s*$/ ||
($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
(defined($1) || defined($2))))) {
- $is_patch = 1;
$reported_maintainer_file = 1;
WARN("FILE_PATH_CHANGES",
"added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
@@ -2612,6 +2442,20 @@ sub process {
$herecurr) if (!$emitted_corrupt++);
}
+# Check for absolute kernel paths.
+ if ($tree) {
+ while ($line =~ m{(?:^|\s)(/\S*)}g) {
+ my $file = $1;
+
+ if ($file =~ m{^(.*?)(?::\d+)+:?$} &&
+ check_absolute_file($1, $herecurr)) {
+ #
+ } else {
+ check_absolute_file($file, $herecurr);
+ }
+ }
+ }
+
# UTF-8 regex found at http://www.w3.org/International/questions/qa-forms-utf-8.en.php
if (($realfile =~ /^$/ || $line =~ /^\+/) &&
$rawline !~ m/^$UTF8*$/) {
@@ -2628,11 +2472,10 @@ sub process {
# Check if it's the start of a commit log
# (not a header line and we haven't seen the patch filename)
if ($in_header_lines && $realfile =~ /^$/ &&
- !($rawline =~ /^\s+(?:\S|$)/ ||
- $rawline =~ /^(?:commit\b|from\b|[\w-]+:)/i)) {
+ !($rawline =~ /^\s+\S/ ||
+ $rawline =~ /^(commit\b|from\b|[\w-]+:).*$/i)) {
$in_header_lines = 0;
$in_commit_log = 1;
- $has_commit_log = 1;
}
# Check if there is UTF-8 in a commit log when a mail header has explicitly
@@ -2649,20 +2492,6 @@ sub process {
"8-bit UTF-8 used in possible commit log\n" . $herecurr);
}
-# Check for absolute kernel paths in commit message
- if ($tree && $in_commit_log) {
- while ($line =~ m{(?:^|\s)(/\S*)}g) {
- my $file = $1;
-
- if ($file =~ m{^(.*?)(?::\d+)+:?$} &&
- check_absolute_file($1, $herecurr)) {
- #
- } else {
- check_absolute_file($file, $herecurr);
- }
- }
- }
-
# Check for various typo / spelling mistakes
if (defined($misspellings) &&
($in_commit_log || $line =~ /^(?:\+|Subject:)/i)) {
@@ -2705,7 +2534,6 @@ sub process {
# Check for FSF mailing addresses.
if ($rawline =~ /\bwrite to the Free/i ||
- $rawline =~ /\b675\s+Mass\s+Ave/i ||
$rawline =~ /\b59\s+Temple\s+Pl/i ||
$rawline =~ /\b51\s+Franklin\s+St/i) {
my $herevet = "$here\n" . cat_vet($rawline) . "\n";
@@ -2757,6 +2585,13 @@ sub process {
#print "is_start<$is_start> is_end<$is_end> length<$length>\n";
}
+# discourage the addition of CONFIG_EXPERIMENTAL in Kconfig.
+ if ($realfile =~ /Kconfig/ &&
+ $line =~ /.\s*depends on\s+.*\bEXPERIMENTAL\b/) {
+ WARN("CONFIG_EXPERIMENTAL",
+ "Use of CONFIG_EXPERIMENTAL is deprecated. For alternatives, see https://lkml.org/lkml/2012/10/23/580\n");
+ }
+
# discourage the use of boolean for type definition attributes of Kconfig options
if ($realfile =~ /Kconfig/ &&
$line =~ /^\+\s*\bboolean\b/) {
@@ -2810,7 +2645,7 @@ sub process {
}
# check we are in a valid source file if not then ignore this hunk
- next if ($realfile !~ /\.(h|c|s|S|sh|dtsi|dts)$/);
+ next if ($realfile !~ /\.(h|c|s|S|pl|sh|dtsi|dts)$/);
# line length limit (with some exclusions)
#
@@ -2844,10 +2679,6 @@ sub process {
$line =~ /^\+\s*#\s*define\s+\w+\s+$String$/) {
$msg_type = "";
- # EFI_GUID is another special case
- } elsif ($line =~ /^\+.*\bEFI_GUID\s*\(/) {
- $msg_type = "";
-
# Otherwise set the alternate message types
# a comment starts before $max_line_length
@@ -2923,19 +2754,6 @@ sub process {
"Logical continuations should be on the previous line\n" . $hereprev);
}
-# check indentation starts on a tab stop
- if ($^V && $^V ge 5.10.0 &&
- $sline =~ /^\+\t+( +)(?:$c90_Keywords\b|\{\s*$|\}\s*(?:else\b|while\b|\s*$))/) {
- my $indent = length($1);
- if ($indent % 8) {
- if (WARN("TABSTOP",
- "Statements should start on a tabstop\n" . $herecurr) &&
- $fix) {
- $fixed[$fixlinenr] =~ s@(^\+\t+) +@$1 . "\t" x ($indent/8)@e;
- }
- }
- }
-
# check multi-line statement indentation matches previous line
if ($^V && $^V ge 5.10.0 &&
$prevline =~ /^\+([ \t]*)((?:$c90_Keywords(?:\s+if)\s*)|(?:$Declare\s*)?(?:$Ident|\(\s*\*\s*$Ident\s*\))\s*|$Ident\s*=\s*$Ident\s*)\(.*(\&\&|\|\||,)\s*$/) {
@@ -3012,30 +2830,6 @@ sub process {
"Block comments use a trailing */ on a separate line\n" . $herecurr);
}
-# Block comment * alignment
- if ($prevline =~ /$;[ \t]*$/ && #ends in comment
- $line =~ /^\+[ \t]*$;/ && #leading comment
- $rawline =~ /^\+[ \t]*\*/ && #leading *
- (($prevrawline =~ /^\+.*?\/\*/ && #leading /*
- $prevrawline !~ /\*\/[ \t]*$/) || #no trailing */
- $prevrawline =~ /^\+[ \t]*\*/)) { #leading *
- my $oldindent;
- $prevrawline =~ m@^\+([ \t]*/?)\*@;
- if (defined($1)) {
- $oldindent = expand_tabs($1);
- } else {
- $prevrawline =~ m@^\+(.*/?)\*@;
- $oldindent = expand_tabs($1);
- }
- $rawline =~ m@^\+([ \t]*)\*@;
- my $newindent = $1;
- $newindent = expand_tabs($newindent);
- if (length($oldindent) ne length($newindent)) {
- WARN("BLOCK_COMMENT_STYLE",
- "Block comments should align the * on each line\n" . $hereprev);
- }
- }
-
# check for missing blank lines after struct/union declarations
# with exceptions for various attributes and macros
if ($prevline =~ /^[\+ ]};?\s*$/ &&
@@ -3126,17 +2920,6 @@ sub process {
# check we are in a valid C source file if not then ignore this hunk
next if ($realfile !~ /\.(h|c)$/);
-# check if this appears to be the start function declaration, save the name
- if ($sline =~ /^\+\{\s*$/ &&
- $prevline =~ /^\+(?:(?:(?:$Storage|$Inline)\s*)*\s*$Type\s*)?($Ident)\(/) {
- $context_function = $1;
- }
-
-# check if this appears to be the end of function declaration
- if ($sline =~ /^\+\}\s*$/) {
- undef $context_function;
- }
-
# check indentation of any line with a bare else
# (but not if it is a multiple line "if (foo) return bar; else return baz;")
# if the previous line is a break or return and is indented 1 tab more...
@@ -3161,6 +2944,12 @@ sub process {
}
}
+# discourage the addition of CONFIG_EXPERIMENTAL in #if(def).
+ if ($line =~ /^\+\s*\#\s*if.*\bCONFIG_EXPERIMENTAL\b/) {
+ WARN("CONFIG_EXPERIMENTAL",
+ "Use of CONFIG_EXPERIMENTAL is deprecated. For alternatives, see https://lkml.org/lkml/2012/10/23/580\n");
+ }
+
# check for RCS/CVS revision markers
if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|)/) {
WARN("CVS_KEYWORD",
@@ -3450,42 +3239,6 @@ sub process {
#ignore lines not being added
next if ($line =~ /^[^\+]/);
-# check for dereferences that span multiple lines
- if ($prevline =~ /^\+.*$Lval\s*(?:\.|->)\s*$/ &&
- $line =~ /^\+\s*(?!\#\s*(?!define\s+|if))\s*$Lval/) {
- $prevline =~ /($Lval\s*(?:\.|->))\s*$/;
- my $ref = $1;
- $line =~ /^.\s*($Lval)/;
- $ref .= $1;
- $ref =~ s/\s//g;
- WARN("MULTILINE_DEREFERENCE",
- "Avoid multiple line dereference - prefer '$ref'\n" . $hereprev);
- }
-
-# check for declarations of signed or unsigned without int
- while ($line =~ m{\b($Declare)\s*(?!char\b|short\b|int\b|long\b)\s*($Ident)?\s*[=,;\[\)\(]}g) {
- my $type = $1;
- my $var = $2;
- $var = "" if (!defined $var);
- if ($type =~ /^(?:(?:$Storage|$Inline|$Attribute)\s+)*((?:un)?signed)((?:\s*\*)*)\s*$/) {
- my $sign = $1;
- my $pointer = $2;
-
- $pointer = "" if (!defined $pointer);
-
- if (WARN("UNSPECIFIED_INT",
- "Prefer '" . trim($sign) . " int" . rtrim($pointer) . "' to bare use of '$sign" . rtrim($pointer) . "'\n" . $herecurr) &&
- $fix) {
- my $decl = trim($sign) . " int ";
- my $comp_pointer = $pointer;
- $comp_pointer =~ s/\s//g;
- $decl .= $comp_pointer;
- $decl = rtrim($decl) if ($var eq "");
- $fixed[$fixlinenr] =~ s@\b$sign\s*\Q$pointer\E\s*$var\b@$decl$var@;
- }
- }
- }
-
# TEST: allow direct testing of the type matcher.
if ($dbg_type) {
if ($line =~ /^.\s*$Declare\s*$/) {
@@ -3684,13 +3437,22 @@ sub process {
}
}
+# check for uses of DEFINE_PCI_DEVICE_TABLE
+ if ($line =~ /\bDEFINE_PCI_DEVICE_TABLE\s*\(\s*(\w+)\s*\)\s*=/) {
+ if (WARN("DEFINE_PCI_DEVICE_TABLE",
+ "Prefer struct pci_device_id over deprecated DEFINE_PCI_DEVICE_TABLE\n" . $herecurr) &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/\b(?:static\s+|)DEFINE_PCI_DEVICE_TABLE\s*\(\s*(\w+)\s*\)\s*=\s*/static const struct pci_device_id $1\[\] = /;
+ }
+ }
+
# check for new typedefs, only function parameters and sparse annotations
# make sense.
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/) {
+ $line !~ /\b__bitwise(?:__|)\b/) {
WARN("NEW_TYPEDEFS",
"do not add new typedefs\n" . $herecurr);
}
@@ -4346,7 +4108,7 @@ sub process {
## }
#need space before brace following if, while, etc
- if (($line =~ /\(.*\)\{/ && $line !~ /\($Type\)\{/) ||
+ if (($line =~ /\(.*\)\{/ && $line !~ /\($Type\){/) ||
$line =~ /do\{/) {
if (ERROR("SPACING",
"space required before the open brace '{'\n" . $herecurr) &&
@@ -4504,7 +4266,7 @@ sub process {
my $comp = $3;
my $to = $4;
my $newcomp = $comp;
- if ($lead !~ /(?:$Operators|\.)\s*$/ &&
+ if ($lead !~ /$Operators\s*$/ &&
$to !~ /^(?:Constant|[A-Z_][A-Z0-9_]*)$/ &&
WARN("CONSTANT_COMPARISON",
"Comparisons should place the constant on the right side of the test\n" . $herecurr) &&
@@ -4777,19 +4539,9 @@ sub process {
#print "LINE<$lines[$ln-1]> len<" . length($lines[$ln-1]) . "\n";
$has_flow_statement = 1 if ($ctx =~ /\b(goto|return)\b/);
- $has_arg_concat = 1 if ($ctx =~ /\#\#/ && $ctx !~ /\#\#\s*(?:__VA_ARGS__|args)\b/);
-
- $dstat =~ s/^.\s*\#\s*define\s+$Ident(\([^\)]*\))?\s*//;
- my $define_args = $1;
- my $define_stmt = $dstat;
- my @def_args = ();
-
- if (defined $define_args && $define_args ne "") {
- $define_args = substr($define_args, 1, length($define_args) - 2);
- $define_args =~ s/\s*//g;
- @def_args = split(",", $define_args);
- }
+ $has_arg_concat = 1 if ($ctx =~ /\#\#/);
+ $dstat =~ s/^.\s*\#\s*define\s+$Ident(?:\([^\)]*\))?\s*//;
$dstat =~ s/$;//g;
$dstat =~ s/\\\n.//g;
$dstat =~ s/^\s*//s;
@@ -4798,7 +4550,7 @@ sub process {
# Flatten any parentheses and braces
while ($dstat =~ s/\([^\(\)]*\)/1/ ||
$dstat =~ s/\{[^\{\}]*\}/1/ ||
- $dstat =~ s/.\[[^\[\]]*\]/1/)
+ $dstat =~ s/\[[^\[\]]*\]/1/)
{
}
@@ -4808,9 +4560,6 @@ sub process {
{
}
- # Make asm volatile uses seem like a generic function
- $dstat =~ s/\b_*asm_*\s+_*volatile_*\b/asm_volatile/g;
-
my $exceptions = qr{
$Declare|
module_param_named|
@@ -4821,19 +4570,9 @@ sub process {
union|
struct|
\.$Ident\s*=\s*|
- ^\"|\"$|
- ^\[
+ ^\"|\"$
}x;
#print "REST<$rest> dstat<$dstat> ctx<$ctx>\n";
-
- $ctx =~ s/\n*$//;
- my $herectx = $here . "\n";
- my $stmt_cnt = statement_rawlines($ctx);
-
- for (my $n = 0; $n < $stmt_cnt; $n++) {
- $herectx .= raw_line($linenr, $n) . "\n";
- }
-
if ($dstat ne '' &&
$dstat !~ /^(?:$Ident|-?$Constant),$/ && # 10, // foo(),
$dstat !~ /^(?:$Ident|-?$Constant);$/ && # foo();
@@ -4849,54 +4588,24 @@ sub process {
$dstat !~ /^\(\{/ && # ({...
$ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
{
+ $ctx =~ s/\n*$//;
+ my $herectx = $here . "\n";
+ my $cnt = statement_rawlines($ctx);
- if ($dstat =~ /;/) {
+ for (my $n = 0; $n < $cnt; $n++) {
+ $herectx .= raw_line($linenr, $n) . "\n";
+ }
+
+ if ($dstat =~ /^\s*if\b/) {
+ ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
+ "Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects\n" . "$herectx");
+ } elsif ($dstat =~ /;/) {
ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
"Macros with multiple statements should be enclosed in a do - while loop\n" . "$herectx");
} else {
ERROR("COMPLEX_MACRO",
"Macros with complex values should be enclosed in parentheses\n" . "$herectx");
}
-
- }
-
- # Make $define_stmt single line, comment-free, etc
- my @stmt_array = split('\n', $define_stmt);
- my $first = 1;
- $define_stmt = "";
- foreach my $l (@stmt_array) {
- $l =~ s/\\$//;
- if ($first) {
- $define_stmt = $l;
- $first = 0;
- } elsif ($l =~ /^[\+ ]/) {
- $define_stmt .= substr($l, 1);
- }
- }
- $define_stmt =~ s/$;//g;
- $define_stmt =~ s/\s+/ /g;
- $define_stmt = trim($define_stmt);
-
-# check if any macro arguments are reused (ignore '...' and 'type')
- foreach my $arg (@def_args) {
- next if ($arg =~ /\.\.\./);
- next if ($arg =~ /^type$/i);
- my $tmp = $define_stmt;
- $tmp =~ s/\b(typeof|__typeof__|__builtin\w+|typecheck\s*\(\s*$Type\s*,|\#+)\s*\(*\s*$arg\s*\)*\b//g;
- $tmp =~ s/\#+\s*$arg\b//g;
- $tmp =~ s/\b$arg\s*\#\#//g;
- my $use_cnt = $tmp =~ s/\b$arg\b//g;
- if ($use_cnt > 1) {
- CHK("MACRO_ARG_REUSE",
- "Macro argument reuse '$arg' - possible side-effects?\n" . "$herectx");
- }
-# check if any macro arguments may have other precedence issues
- if ($define_stmt =~ m/($Operators)?\s*\b$arg\b\s*($Operators)?/m &&
- ((defined($1) && $1 ne ',') ||
- (defined($2) && $2 ne ','))) {
- CHK("MACRO_ARG_PRECEDENCE",
- "Macro argument '$arg' may be better as '($arg)' to avoid precedence issues\n" . "$herectx");
- }
}
# check for macros with flow control, but without ## concatenation
@@ -5104,12 +4813,6 @@ sub process {
}
}
-# check for single line unbalanced braces
- if ($sline =~ /^.\s*\}\s*else\s*$/ ||
- $sline =~ /^.\s*else\s*\{\s*$/) {
- CHK("BRACES", "Unbalanced braces around else statement\n" . $herecurr);
- }
-
# check for unnecessary blank lines around braces
if (($line =~ /^.\s*}\s*$/ && $prevrawline =~ /^.\s*$/)) {
if (CHK("BRACES",
@@ -5130,7 +4833,7 @@ sub process {
my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
WARN("VOLATILE",
- "Use of volatile is usually wrong: see Documentation/process/volatile-considered-harmful.rst\n" . $herecurr);
+ "Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
}
# Check for user-visible strings broken across lines, which breaks the ability
@@ -5172,18 +4875,6 @@ sub process {
"break quoted strings at a space character\n" . $hereprev);
}
-# check for an embedded function name in a string when the function is known
-# This does not work very well for -f --file checking as it depends on patch
-# context providing the function name or a single line form for in-file
-# function declarations
- if ($line =~ /^\+.*$String/ &&
- defined($context_function) &&
- get_quoted_string($line, $rawline) =~ /\b$context_function\b/ &&
- length(get_quoted_string($line, $rawline)) != (length($context_function) + 2)) {
- WARN("EMBEDDED_FUNCTION_NAME",
- "Prefer using '\"%s...\", __func__' to using '$context_function', this function's name, in a string\n" . $herecurr);
- }
-
# check for spaces before a quoted newline
if ($rawline =~ /^.*\".*\s\\n/) {
if (WARN("QUOTED_WHITESPACE_BEFORE_NEWLINE",
@@ -5206,27 +4897,18 @@ sub process {
"Consecutive strings are generally better as a single string\n" . $herecurr);
}
-# check for non-standard and hex prefixed decimal printf formats
- my $show_L = 1; #don't show the same defect twice
- my $show_Z = 1;
+# check for %L{u,d,i} and 0x%[udi] in strings
+ my $string;
while ($line =~ /(?:^|")([X\t]*)(?:"|$)/g) {
- my $string = substr($rawline, $-[1], $+[1] - $-[1]);
+ $string = substr($rawline, $-[1], $+[1] - $-[1]);
$string =~ s/%%/__/g;
- # check for %L
- if ($show_L && $string =~ /%[\*\d\.\$]*L([diouxX])/) {
+ if ($string =~ /(?<!%)%[\*\d\.\$]*L[udi]/) {
WARN("PRINTF_L",
- "\%L$1 is non-standard C, use %ll$1\n" . $herecurr);
- $show_L = 0;
- }
- # check for %Z
- if ($show_Z && $string =~ /%[\*\d\.\$]*Z([diouxX])/) {
- WARN("PRINTF_Z",
- "%Z$1 is non-standard C, use %z$1\n" . $herecurr);
- $show_Z = 0;
- }
- # check for 0x<decimal>
- if ($string =~ /0x%[\*\d\.\$\Llzth]*[diou]/) {
- ERROR("PRINTF_0XDECIMAL",
+ "\%Ld/%Lu are not-standard C, use %lld/%llu\n" . $herecurr);
+ last;
+ }
+ if ($string =~ /0x%[\*\d\.\$\Llzth]*[udi]/) {
+ ERROR("PRINTF_0xDECIMAL",
"Prefixing 0x with decimal output is defective\n" . $herecurr);
}
}
@@ -5305,12 +4987,6 @@ sub process {
}
}
-# check for logging continuations
- if ($line =~ /\bprintk\s*\(\s*KERN_CONT\b|\bpr_cont\s*\(/) {
- WARN("LOGGING_CONTINUATION",
- "Avoid logging continuation uses where feasible\n" . $herecurr);
- }
-
# check for mask then right shift without a parentheses
if ($^V && $^V ge 5.10.0 &&
$line =~ /$LvalOrFunc\s*\&\s*($LvalOrFunc)\s*>>/ &&
@@ -5603,9 +5279,8 @@ sub process {
"Using weak declarations can have unintended link defects\n" . $herecurr);
}
-# check for c99 types like uint8_t used outside of uapi/ and tools/
+# check for c99 types like uint8_t used outside of uapi/
if ($realfile !~ m@\binclude/uapi/@ &&
- $realfile !~ m@\btools/@ &&
$line =~ /\b($Declare)\s*$Ident\s*[=;,\[]/) {
my $type = $1;
if ($type =~ /\b($typeC99Typedefs)\b/) {
@@ -5676,32 +5351,6 @@ sub process {
}
}
- # check for vsprintf extension %p<foo> misuses
- if ($^V && $^V ge 5.10.0 &&
- defined $stat &&
- $stat =~ /^\+(?![^\{]*\{\s*).*\b(\w+)\s*\(.*$String\s*,/s &&
- $1 !~ /^_*volatile_*$/) {
- my $bad_extension = "";
- my $lc = $stat =~ tr@\n@@;
- $lc = $lc + $linenr;
- for (my $count = $linenr; $count <= $lc; $count++) {
- my $fmt = get_quoted_string($lines[$count - 1], raw_line($count, 0));
- $fmt =~ s/%%//g;
- if ($fmt =~ /(\%[\*\d\.]*p(?![\WFfSsBKRraEhMmIiUDdgVCbGN]).)/) {
- $bad_extension = $1;
- last;
- }
- }
- if ($bad_extension ne "") {
- my $stat_real = raw_line($linenr, 0);
- for (my $count = $linenr + 1; $count <= $lc; $count++) {
- $stat_real = $stat_real . "\n" . raw_line($count, 0);
- }
- WARN("VSPRINTF_POINTER_EXTENSION",
- "Invalid vsprintf pointer extension '$bad_extension'\n" . "$here\n$stat_real\n");
- }
- }
-
# Check for misused memsets
if ($^V && $^V ge 5.10.0 &&
defined $stat &&
@@ -5721,46 +5370,46 @@ sub process {
}
# Check for memcpy(foo, bar, ETH_ALEN) that could be ether_addr_copy(foo, bar)
-# if ($^V && $^V ge 5.10.0 &&
-# defined $stat &&
-# $stat =~ /^\+(?:.*?)\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) {
-# if (WARN("PREFER_ETHER_ADDR_COPY",
-# "Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2)\n" . "$here\n$stat\n") &&
-# $fix) {
-# $fixed[$fixlinenr] =~ s/\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/ether_addr_copy($2, $7)/;
-# }
-# }
+ if ($^V && $^V ge 5.10.0 &&
+ defined $stat &&
+ $stat =~ /^\+(?:.*?)\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) {
+ if (WARN("PREFER_ETHER_ADDR_COPY",
+ "Prefer ether_addr_copy() over memcpy() if the Ethernet addresses are __aligned(2)\n" . "$here\n$stat\n") &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/\bmemcpy\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/ether_addr_copy($2, $7)/;
+ }
+ }
# Check for memcmp(foo, bar, ETH_ALEN) that could be ether_addr_equal*(foo, bar)
-# if ($^V && $^V ge 5.10.0 &&
-# defined $stat &&
-# $stat =~ /^\+(?:.*?)\bmemcmp\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) {
-# WARN("PREFER_ETHER_ADDR_EQUAL",
-# "Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()\n" . "$here\n$stat\n")
-# }
+ if ($^V && $^V ge 5.10.0 &&
+ defined $stat &&
+ $stat =~ /^\+(?:.*?)\bmemcmp\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) {
+ WARN("PREFER_ETHER_ADDR_EQUAL",
+ "Prefer ether_addr_equal() or ether_addr_equal_unaligned() over memcmp()\n" . "$here\n$stat\n")
+ }
# check for memset(foo, 0x0, ETH_ALEN) that could be eth_zero_addr
# check for memset(foo, 0xFF, ETH_ALEN) that could be eth_broadcast_addr
-# if ($^V && $^V ge 5.10.0 &&
-# defined $stat &&
-# $stat =~ /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) {
-#
-# my $ms_val = $7;
-#
-# if ($ms_val =~ /^(?:0x|)0+$/i) {
-# if (WARN("PREFER_ETH_ZERO_ADDR",
-# "Prefer eth_zero_addr over memset()\n" . "$here\n$stat\n") &&
-# $fix) {
-# $fixed[$fixlinenr] =~ s/\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*,\s*ETH_ALEN\s*\)/eth_zero_addr($2)/;
-# }
-# } elsif ($ms_val =~ /^(?:0xff|255)$/i) {
-# if (WARN("PREFER_ETH_BROADCAST_ADDR",
-# "Prefer eth_broadcast_addr() over memset()\n" . "$here\n$stat\n") &&
-# $fix) {
-# $fixed[$fixlinenr] =~ s/\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*,\s*ETH_ALEN\s*\)/eth_broadcast_addr($2)/;
-# }
-# }
-# }
+ if ($^V && $^V ge 5.10.0 &&
+ defined $stat &&
+ $stat =~ /^\+(?:.*?)\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*\,\s*ETH_ALEN\s*\)/) {
+
+ my $ms_val = $7;
+
+ if ($ms_val =~ /^(?:0x|)0+$/i) {
+ if (WARN("PREFER_ETH_ZERO_ADDR",
+ "Prefer eth_zero_addr over memset()\n" . "$here\n$stat\n") &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*,\s*ETH_ALEN\s*\)/eth_zero_addr($2)/;
+ }
+ } elsif ($ms_val =~ /^(?:0xff|255)$/i) {
+ if (WARN("PREFER_ETH_BROADCAST_ADDR",
+ "Prefer eth_broadcast_addr() over memset()\n" . "$here\n$stat\n") &&
+ $fix) {
+ $fixed[$fixlinenr] =~ s/\bmemset\s*\(\s*$FuncArg\s*,\s*$FuncArg\s*,\s*ETH_ALEN\s*\)/eth_broadcast_addr($2)/;
+ }
+ }
+ }
# typecasts on min/max could be min_t/max_t
if ($^V && $^V ge 5.10.0 &&
@@ -5880,26 +5529,13 @@ sub process {
"externs should be avoided in .c files\n" . $herecurr);
}
- if ($realfile =~ /\.[ch]$/ && defined $stat &&
- $stat =~ /^.\s*(?:extern\s+)?$Type\s*$Ident\s*\(\s*([^{]+)\s*\)\s*;/s &&
- $1 ne "void") {
- my $args = trim($1);
- while ($args =~ m/\s*($Type\s*(?:$Ident|\(\s*\*\s*$Ident?\s*\)\s*$balanced_parens)?)/g) {
- my $arg = trim($1);
- if ($arg =~ /^$Type$/ && $arg !~ /enum\s+$Ident$/) {
- WARN("FUNCTION_ARGUMENTS",
- "function definition argument '$arg' should also have an identifier name\n" . $herecurr);
- }
- }
- }
-
# checks for new __setup's
if ($rawline =~ /\b__setup\("([^"]*)"/) {
my $name = $1;
if (!grep(/$name/, @setup_docs)) {
CHK("UNDOCUMENTED_SETUP",
- "__setup appears un-documented -- check Documentation/admin-guide/kernel-parameters.rst\n" . $herecurr);
+ "__setup appears un-documented -- check Documentation/kernel-parameters.txt\n" . $herecurr);
}
}
@@ -5964,9 +5600,8 @@ sub process {
}
}
-# check for #defines like: 1 << <digit> that could be BIT(digit), it is not exported to uapi
- if ($realfile !~ m@^include/uapi/@ &&
- $line =~ /#\s*define\s+\w+\s+\(?\s*1\s*([ulUL]*)\s*\<\<\s*(?:\d+|$Ident)\s*\)?/) {
+# check for #defines like: 1 << <digit> that could be BIT(digit)
+ if ($line =~ /#\s*define\s+\w+\s+\(?\s*1\s*([ulUL]*)\s*\<\<\s*(?:\d+|$Ident)\s*\)?/) {
my $ull = "";
$ull = "_ULL" if (defined($1) && $1 =~ /ll/i);
if (CHK("BIT_MACRO",
@@ -5976,16 +5611,6 @@ sub process {
}
}
-# check for #if defined CONFIG_<FOO> || defined CONFIG_<FOO>_MODULE
- if ($line =~ /^\+\s*#\s*if\s+defined(?:\s*\(?\s*|\s+)(CONFIG_[A-Z_]+)\s*\)?\s*\|\|\s*defined(?:\s*\(?\s*|\s+)\1_MODULE\s*\)?\s*$/) {
- my $config = $1;
- if (WARN("PREFER_IS_ENABLED",
- "Prefer IS_ENABLED(<FOO>) to CONFIG_<FOO> || CONFIG_<FOO>_MODULE\n" . $herecurr) &&
- $fix) {
- $fixed[$fixlinenr] = "\+#if IS_ENABLED($config)";
- }
- }
-
# check for case / default statements not preceded by break/fallthrough/switch
if ($line =~ /^.\s*(?:case\s+(?:$Ident|$Constant)\s*|default):/) {
my $has_break = 0;
@@ -6007,7 +5632,7 @@ sub process {
}
if (!$has_break && $has_statement) {
WARN("MISSING_BREAK",
- "Possible switch case/default not preceded by break or fallthrough comment\n" . $herecurr);
+ "Possible switch case/default not preceeded by break or fallthrough comment\n" . $herecurr);
}
}
@@ -6092,6 +5717,46 @@ sub process {
}
# check for various structs that are normally const (ops, kgdb, device_tree)
+ my $const_structs = qr{
+ acpi_dock_ops|
+ address_space_operations|
+ backlight_ops|
+ block_device_operations|
+ dentry_operations|
+ dev_pm_ops|
+ dma_map_ops|
+ extent_io_ops|
+ file_lock_operations|
+ file_operations|
+ hv_ops|
+ ide_dma_ops|
+ intel_dvo_dev_ops|
+ item_operations|
+ iwl_ops|
+ kgdb_arch|
+ kgdb_io|
+ kset_uevent_ops|
+ lock_manager_operations|
+ microcode_ops|
+ mtrr_ops|
+ neigh_ops|
+ nlmsvc_binding|
+ of_device_id|
+ pci_raw_ops|
+ pipe_buf_operations|
+ platform_hibernation_ops|
+ platform_suspend_ops|
+ proto_ops|
+ rpc_pipe_ops|
+ seq_operations|
+ snd_ac97_build_ops|
+ soc_pcmcia_socket_ops|
+ stacktrace_ops|
+ sysfs_ops|
+ tty_operations|
+ uart_ops|
+ usb_mon_operations|
+ wd_ops}x;
if ($line !~ /\bconst\b/ &&
$line =~ /\bstruct\s+($const_structs)\b/) {
WARN("CONST_STRUCT",
@@ -6136,34 +5801,6 @@ sub process {
}
}
-# whine about ACCESS_ONCE
- if ($^V && $^V ge 5.10.0 &&
- $line =~ /\bACCESS_ONCE\s*$balanced_parens\s*(=(?!=))?\s*($FuncArg)?/) {
- my $par = $1;
- my $eq = $2;
- my $fun = $3;
- $par =~ s/^\(\s*(.*)\s*\)$/$1/;
- if (defined($eq)) {
- if (WARN("PREFER_WRITE_ONCE",
- "Prefer WRITE_ONCE(<FOO>, <BAR>) over ACCESS_ONCE(<FOO>) = <BAR>\n" . $herecurr) &&
- $fix) {
- $fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)\s*$eq\s*\Q$fun\E/WRITE_ONCE($par, $fun)/;
- }
- } else {
- if (WARN("PREFER_READ_ONCE",
- "Prefer READ_ONCE(<FOO>) over ACCESS_ONCE(<FOO>)\n" . $herecurr) &&
- $fix) {
- $fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)/READ_ONCE($par)/;
- }
- }
- }
-
-# check for mutex_trylock_recursive usage
- if ($line =~ /mutex_trylock_recursive/) {
- ERROR("LOCKING",
- "recursive locking is bad, do not use this ever.\n" . $herecurr);
- }
-
# check for lockdep_set_novalidate_class
if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
$line =~ /__lockdep_no_validate__\s*\)/ ) {
@@ -6184,69 +5821,34 @@ sub process {
# Mode permission misuses where it seems decimal should be octal
# This uses a shortcut match to avoid unnecessary uses of a slow foreach loop
if ($^V && $^V ge 5.10.0 &&
- defined $stat &&
$line =~ /$mode_perms_search/) {
foreach my $entry (@mode_permission_funcs) {
my $func = $entry->[0];
my $arg_pos = $entry->[1];
- my $lc = $stat =~ tr@\n@@;
- $lc = $lc + $linenr;
- my $stat_real = raw_line($linenr, 0);
- for (my $count = $linenr + 1; $count <= $lc; $count++) {
- $stat_real = $stat_real . "\n" . raw_line($count, 0);
- }
-
my $skip_args = "";
if ($arg_pos > 1) {
$arg_pos--;
$skip_args = "(?:\\s*$FuncArg\\s*,\\s*){$arg_pos,$arg_pos}";
}
- my $test = "\\b$func\\s*\\(${skip_args}($FuncArg(?:\\|\\s*$FuncArg)*)\\s*[,\\)]";
- if ($stat =~ /$test/) {
+ my $test = "\\b$func\\s*\\(${skip_args}([\\d]+)\\s*[,\\)]";
+ if ($line =~ /$test/) {
my $val = $1;
$val = $6 if ($skip_args ne "");
- if (($val =~ /^$Int$/ && $val !~ /^$Octal$/) ||
- ($val =~ /^$Octal$/ && length($val) ne 4)) {
+
+ if ($val !~ /^0$/ &&
+ (($val =~ /^$Int$/ && $val !~ /^$Octal$/) ||
+ length($val) ne 4)) {
ERROR("NON_OCTAL_PERMISSIONS",
- "Use 4 digit octal (0777) not decimal permissions\n" . "$here\n" . $stat_real);
- }
- if ($val =~ /^$Octal$/ && (oct($val) & 02)) {
+ "Use 4 digit octal (0777) not decimal permissions\n" . $herecurr);
+ } elsif ($val =~ /^$Octal$/ && (oct($val) & 02)) {
ERROR("EXPORTED_WORLD_WRITABLE",
- "Exporting writable files is usually an error. Consider more restrictive permissions.\n" . "$here\n" . $stat_real);
+ "Exporting writable files is usually an error. Consider more restrictive permissions.\n" . $herecurr);
}
}
}
}
-# check for uses of S_<PERMS> that could be octal for readability
- if ($line =~ /\b$mode_perms_string_search\b/) {
- my $val = "";
- my $oval = "";
- my $to = 0;
- my $curpos = 0;
- my $lastpos = 0;
- while ($line =~ /\b(($mode_perms_string_search)\b(?:\s*\|\s*)?\s*)/g) {
- $curpos = pos($line);
- my $match = $2;
- my $omatch = $1;
- last if ($lastpos > 0 && ($curpos - length($omatch) != $lastpos));
- $lastpos = $curpos;
- $to |= $mode_permission_string_types{$match};
- $val .= '\s*\|\s*' if ($val ne "");
- $val .= $match;
- $oval .= $omatch;
- }
- $oval =~ s/^\s*\|\s*//;
- $oval =~ s/\s*\|\s*$//;
- my $octal = sprintf("%04o", $to);
- if (WARN("SYMBOLIC_PERMS",
- "Symbolic permissions '$oval' are not preferred. Consider using octal permissions '$octal'.\n" . $herecurr) &&
- $fix) {
- $fixed[$fixlinenr] =~ s/$val/$octal/;
- }
- }
-
# validate content of MODULE_LICENSE against list from include/linux/module.h
if ($line =~ /\bMODULE_LICENSE\s*\(\s*($String)\s*\)/) {
my $extracted_string = get_quoted_string($line, $rawline);
@@ -6288,7 +5890,7 @@ sub process {
ERROR("NOT_UNIFIED_DIFF",
"Does not appear to be a unified-diff format patch\n");
}
- if ($is_patch && $has_commit_log && $chk_signoff && $signoff == 0) {
+ if ($is_patch && $filename ne '-' && $chk_signoff && $signoff == 0) {
ERROR("MISSING_SIGN_OFF",
"Missing Signed-off-by: line(s)\n");
}
@@ -6302,14 +5904,6 @@ sub process {
}
if ($quiet == 0) {
- # If there were any defects found and not already fixing them
- if (!$clean and !$fix) {
- print << "EOM"
-
-NOTE: For some of the reported defects, checkpatch may be able to
- mechanically convert to the typical style using --fix or --fix-inplace.
-EOM
- }
# If there were whitespace errors which cleanpatch can fix
# then suggest that.
if ($rpt_cleaners) {
--
2.10.0.rc2.1.g053435c
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test
2017-04-09 17:45 ` [PATCH 2/2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test Joe Perches
@ 2017-04-09 17:46 ` Joe Perches
2017-04-09 17:59 ` [PATCH V2] " Joe Perches
1 sibling, 0 replies; 8+ messages in thread
From: Joe Perches @ 2017-04-09 17:46 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: Alfonso Lima, Andreas Mohr, linux-kernel
On Sun, 2017-04-09 at 10:45 -0700, Joe Perches wrote:
> checkpatch
Sorry, ignore this.
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH V2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test
2017-04-09 17:45 ` [PATCH 2/2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test Joe Perches
2017-04-09 17:46 ` Joe Perches
@ 2017-04-09 17:59 ` Joe Perches
2017-04-10 0:28 ` [PATCH V3] " Joe Perches
1 sibling, 1 reply; 8+ messages in thread
From: Joe Perches @ 2017-04-09 17:59 UTC (permalink / raw)
To: Andy Whitcroft; +Cc: Alfonso Lima, Andreas Mohr, linux-kernel
The logic currrently misses macros that start with an if statement.
e.g.: #define foo(bar) if (bar) baz;
Add a test for macro content that starts with if
Original-patch-by: Alfonso Lima <alfonsolimaastor@gmail.com>
Reported-by: Andreas Mohr <andi@lisas.de>
Signed-off-by: Joe Perches <joe@perches.com>
---
V2: V1 was done incorrectly against a much older version of checkpatch
and so rolled back several improvements.
I forgot I had used git checkout <commit_id> -- scripts/checkpatch.pl
and then edited that.
scripts/checkpatch.pl | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e8d8481b24c8..2468ed01b333 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4849,8 +4849,10 @@ sub process {
$dstat !~ /^\(\{/ && # ({...
$ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
{
-
- if ($dstat =~ /;/) {
+ if ($dstat =~ /^\s*if\b/) {
+ ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
+ "Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects\n" . "$herectx");
+ elsif ($dstat =~ /;/) {
ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
"Macros with multiple statements should be enclosed in a do - while loop\n" . "$herectx");
} else {
--
2.10.0.rc2.1.g053435c
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH V3] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test
2017-04-09 17:59 ` [PATCH V2] " Joe Perches
@ 2017-04-10 0:28 ` Joe Perches
0 siblings, 0 replies; 8+ messages in thread
From: Joe Perches @ 2017-04-10 0:28 UTC (permalink / raw)
To: Andrew Morton, Andy Whitcroft; +Cc: Alfonso Lima, Andreas Mohr, linux-kernel
The logic currrently misses macros that start with an if statement.
e.g.: #define foo(bar) if (bar) baz;
Add a test for macro content that starts with if
Original-patch-by: Alfonso Lima <alfonsolimaastor@gmail.com>
Reported-by: Andreas Mohr <andi@lisas.de>
Signed-off-by: Joe Perches <joe@perches.com>
---
V3: Fix bad merge - Add missing closing brace
V2: V1 was done incorrectly against a much older version of checkpatch
and so rolled back several improvements.
I forgot I had used git checkout <commit_id> -- scripts/checkpatch.pl
and then edited that.
scripts/checkpatch.pl | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index e8d8481b24c8..0a26f8ada9f9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4849,8 +4849,10 @@ sub process {
$dstat !~ /^\(\{/ && # ({...
$ctx !~ /^.\s*#\s*define\s+TRACE_(?:SYSTEM|INCLUDE_FILE|INCLUDE_PATH)\b/)
{
-
- if ($dstat =~ /;/) {
+ if ($dstat =~ /^\s*if\b/) {
+ ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
+ "Macros starting with if should be enclosed by a do - while loop to avoid possible if/else logic defects\n" . "$herectx");
+ } elsif ($dstat =~ /;/) {
ERROR("MULTISTATEMENT_MACRO_USE_DO_WHILE",
"Macros with multiple statements should be enclosed in a do - while loop\n" . "$herectx");
} else {
--
2.10.0.rc2.1.g053435c
^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-10 0:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-08 16:07 [bug report] checkpatch: if statement does not need to be enclosed in parentheses Alfonso Lima
2017-04-08 17:33 ` Joe Perches
2017-04-09 8:55 ` Andreas Mohr
2017-04-09 17:45 ` [PATCH 1/2] checkpatch: Clarify the EMBEDDED_FUNCTION_NAME message Joe Perches
2017-04-09 17:45 ` [PATCH 2/2] checkpatch: Improve MULTISTATEMENT_MACRO_USE_DO_WHILE test Joe Perches
2017-04-09 17:46 ` Joe Perches
2017-04-09 17:59 ` [PATCH V2] " Joe Perches
2017-04-10 0:28 ` [PATCH V3] " Joe Perches
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).