From 2d6327459ec5e63a89b12945f483f6d7378a8839 Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:00 -0700 Subject: checkpatch: add PREFER_IS_ENABLED test Using #if defined CONFIG_ || defined CONFIG__MODULE is more verbose than necessary and IS_ENABLED(CONFIG_) is preferred. So add a test and a message for it. --fix it to if desired. Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index d574d13ba963..eb8f88787e81 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5637,6 +5637,16 @@ sub process { } } +# check for #if defined CONFIG_ || defined CONFIG__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() to CONFIG_ || CONFIG__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; -- cgit v1.2.3-59-g8ed1b From f39e1769bbfc4936ff8364fb2529dc8bf6bc6888 Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:02 -0700 Subject: checkpatch: improve CONSTANT_COMPARISON test for structure members A "." dereference to an all uppercase structure member can be incorrectly reported as a CONSTANT_COMPARISON. ie: "if (table[i].PANELID == tempdx)" Fix it by checking for "." before the constant test. Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index eb8f88787e81..e3d9c34b1b53 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -4291,7 +4291,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) && -- cgit v1.2.3-59-g8ed1b From a91e8994f242a500bf05b9ee96fcd7ab0228f05f Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:05 -0700 Subject: checkpatch: add test for keywords not starting on tabstops It's somewhat common and in general a defect for c90 keywords to not start on a tabstop. Add a test for this condition and warn when it occurs. Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index e3d9c34b1b53..6c1213cb32c4 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -2755,6 +2755,19 @@ 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*$/) { -- cgit v1.2.3-59-g8ed1b From 481aea5c59a57123b66d5850be1be79f9f230c0e Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:08 -0700 Subject: checkpatch: whine about ACCESS_ONCE Add a test for use of ACCESS_ONCE that could be written using READ_ONCE or WRITE_ONCE. --fix it too if desired. The WRITE_ONCE fixes are less correct than the coccinelle script below as checkpatch cannot have a completely correct "expression" mechanism because checkpatch works on patches and not complete files. $ cat access_once.cocci @@ expression e1; expression e2; @@ - ACCESS_ONCE(e1) = e2 + WRITE_ONCE(e1, e2) @@ expression e1; @@ - ACCESS_ONCE(e1) + READ_ONCE(e1) Signed-off-by: Joe Perches Cc: Julia Lawall Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 6c1213cb32c4..1048672c22ee 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5850,6 +5850,28 @@ 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(, ) over ACCESS_ONCE() = \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() over ACCESS_ONCE()\n" . $herecurr) && + $fix) { + $fixed[$fixlinenr] =~ s/\bACCESS_ONCE\s*\(\s*\Q$par\E\s*\)/READ_ONCE($par)/; + } + } + } + # check for lockdep_set_novalidate_class if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ || $line =~ /__lockdep_no_validate__\s*\)/ ) { -- cgit v1.2.3-59-g8ed1b From ef212196369cbc2e694eab39261f9785ec252028 Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:11 -0700 Subject: checkpatch: advertise the --fix and --fix-inplace options more The --fix option is relatively unknown and underutilized. Add some text to show that it's available when style defects are found. Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 1048672c22ee..f09c3f28b0fe 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -5975,6 +5975,14 @@ 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) { -- cgit v1.2.3-59-g8ed1b From 3beb42eced39c00011ba4d608d52718af765e5d4 Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:14 -0700 Subject: checkpatch: add --list-types to show message types to show or ignore The message types are not currently knowable without reading the code. Add a mechanism to see what they are. Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index f09c3f28b0fe..c5a3c9513419 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -33,6 +33,7 @@ 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; @@ -70,11 +71,12 @@ Options: --showfile emit diffed file position, not input file position -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 @@ -106,6 +108,37 @@ 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; @@ -146,6 +179,7 @@ GetOptions( '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, @@ -166,6 +200,8 @@ GetOptions( help(0) if ($help); +list_types(0) if ($list_types); + $fix = 1 if ($fix_inplace); $check_orig = $check; -- cgit v1.2.3-59-g8ed1b From 4a593c3448312906358b00898c29a95278d82cc9 Mon Sep 17 00:00:00 2001 From: "Du, Changbin" Date: Fri, 20 May 2016 17:04:16 -0700 Subject: checkpatch: add support to check already applied git commits It's sometimes useful to scan already committed patches. Add --git to scan specific or multiple commits. Single commits are scanned with --git Multiple commits are scanned with --git --git - [joe@perches.com: o Don't exec git for each -, use a single "git log - " o Consolidate the git exec for the and - variants o Output 12 character commit hash ids o Don't scan git commit merges o Use -M to reduce the size of rename commits] Signed-off-by: "Du, Changbin" Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 48 +++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index c5a3c9513419..8fc9edd3289a 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -27,6 +27,7 @@ my $emacs = 0; my $terse = 0; my $showfile = 0; my $file = 0; +my $git = 0; my $check = 0; my $check_orig = 0; my $summary = 1; @@ -69,6 +70,16 @@ 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: + + ^ + ~n + multiple git commits with: + .. + ... + - + 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 @@ -174,6 +185,7 @@ GetOptions( 'terse!' => \$terse, 'showfile!' => \$showfile, 'f|file!' => \$file, + 'g|git!' => \$git, 'subjective!' => \$check, 'strict!' => \$check, 'ignore=s' => \@ignore, @@ -788,10 +800,42 @@ 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 = (); + for my $commit_expr (@ARGV) { + my $git_range; + if ($commit_expr =~ m/-/) { + my @tmp = split(/-/, $commit_expr); + die "$P: incorrect git commits expression $commit_expr$!\n" + if (@tmp != 2); + $git_range = "-$tmp[1] $tmp[0]"; + } elsif ($commit_expr =~ m/\.\./) { + $git_range = "$commit_expr"; + } + if (defined $git_range) { + my $lines = `git log --no-merges --pretty=format:'%H' $git_range`; + foreach my $line (split(/\n/, $lines)) { + unshift(@commits, $line); + } + } else { + unshift(@commits, $commit_expr); + } + } + die "$P: no git commits after extraction!\n" if (@commits == 0); + @ARGV = @commits; +} + my $vname; for my $filename (@ARGV) { my $FILE; - if ($file) { + if ($git) { + open($FILE, '-|', "git format-patch -M --stdout -1 $filename") || + die "$P: $filename: git format-patch failed - $!\n"; + } elsif ($file) { open($FILE, '-|', "diff -u /dev/null $filename") || die "$P: $filename: diff failed - $!\n"; } elsif ($filename eq '-') { @@ -802,6 +846,8 @@ for my $filename (@ARGV) { } if ($filename eq '-') { $vname = 'Your patch'; + } elsif ($git) { + $vname = "Commit " . substr($filename, 0, 12) . `git log -1 --pretty=format:' ("%s")' $filename`; } else { $vname = $filename; } -- cgit v1.2.3-59-g8ed1b From 0dea9f1eef86bedacad91b6f652ca1ab0d08854c Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:19 -0700 Subject: checkpatch: reduce number of `git log` calls with --git checkpatch currently calls git log multiple times to first get the sha1 values and again to get the subject for each individual sha1 commit. Always get the sha1 and subject at the same time instead. Store the subject in a sha1 hash to avoid the second git log exec. Link: http://lkml.kernel.org/r/274efab2332ad2308ab5de85a95d255f6e2de5f3.1462711962.git.joe@perches.com Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 23 +++++++++++++---------- 1 file changed, 13 insertions(+), 10 deletions(-) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 8fc9edd3289a..928366215fc5 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -28,6 +28,7 @@ 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; @@ -806,23 +807,25 @@ die "$P: No git repository found\n" if ($git && !-e ".git"); if ($git) { my @commits = (); - for my $commit_expr (@ARGV) { + foreach my $commit_expr (@ARGV) { my $git_range; if ($commit_expr =~ m/-/) { my @tmp = split(/-/, $commit_expr); - die "$P: incorrect git commits expression $commit_expr$!\n" + die "$P: incorrect git commit expression '$commit_expr' $!\n" if (@tmp != 2); $git_range = "-$tmp[1] $tmp[0]"; } elsif ($commit_expr =~ m/\.\./) { $git_range = "$commit_expr"; - } - if (defined $git_range) { - my $lines = `git log --no-merges --pretty=format:'%H' $git_range`; - foreach my $line (split(/\n/, $lines)) { - unshift(@commits, $line); - } } else { - unshift(@commits, $commit_expr); + $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 =~ /(^\w+) (.*)/; + my $sha1 = $1; + my $subject = $2; + unshift(@commits, $sha1); + $git_commits{$sha1} = $subject; } } die "$P: no git commits after extraction!\n" if (@commits == 0); @@ -847,7 +850,7 @@ for my $filename (@ARGV) { if ($filename eq '-') { $vname = 'Your patch'; } elsif ($git) { - $vname = "Commit " . substr($filename, 0, 12) . `git log -1 --pretty=format:' ("%s")' $filename`; + $vname = "Commit " . substr($filename, 0, 12) . ' ("' . $git_commits{$filename} . '")'; } else { $vname = $filename; } -- cgit v1.2.3-59-g8ed1b From 28898fd1a85fa37a59c090271e8be37afe710276 Mon Sep 17 00:00:00 2001 From: Joe Perches Date: Fri, 20 May 2016 17:04:22 -0700 Subject: checkpatch: improve --git shortcut The --git shortcut can be confused by a tag with a dash like v4.4-rc1. Improve the test to verify the expression ends with a dash followed by a numeric value. Improve the git log result to verify the " " output as well. Link: http://lkml.kernel.org/r/c4a3f759291d967641860c3a54bb81177f34325f.1462711962.git.joe@perches.com Signed-off-by: Joe Perches Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- scripts/checkpatch.pl | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) (limited to 'scripts') diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl index 928366215fc5..6750595bd7b8 100755 --- a/scripts/checkpatch.pl +++ b/scripts/checkpatch.pl @@ -809,11 +809,8 @@ if ($git) { my @commits = (); foreach my $commit_expr (@ARGV) { my $git_range; - if ($commit_expr =~ m/-/) { - my @tmp = split(/-/, $commit_expr); - die "$P: incorrect git commit expression '$commit_expr' $!\n" - if (@tmp != 2); - $git_range = "-$tmp[1] $tmp[0]"; + if ($commit_expr =~ m/^(.*)-(\d+)$/) { + $git_range = "-$2 $1"; } elsif ($commit_expr =~ m/\.\./) { $git_range = "$commit_expr"; } else { @@ -821,7 +818,8 @@ if ($git) { } my $lines = `git log --no-color --no-merges --pretty=format:'%H %s' $git_range`; foreach my $line (split(/\n/, $lines)) { - $line =~ /(^\w+) (.*)/; + $line =~ /^([0-9a-fA-F]{40,40}) (.*)$/; + next if (!defined($1) || !defined($2)); my $sha1 = $1; my $subject = $2; unshift(@commits, $sha1); -- cgit v1.2.3-59-g8ed1b