Patchwork [2/2] checkpatch: adjust to QEMUisms

login
register
mail settings
Submitter Blue Swirl
Date Jan. 9, 2011, 1:24 p.m.
Message ID <AANLkTik_jC62pdS9RR-WPOm2iPFxmXhrxB52ZfzBh5rU@mail.gmail.com>
Download mbox | patch
Permalink /patch/78017/
State New
Headers show

Comments

Blue Swirl - Jan. 9, 2011, 1:24 p.m.
On Sun, Jan 9, 2011 at 1:01 PM, Stuart Brady <sdb@zubnet.me.uk> wrote:
> On Sun, Jan 09, 2011 at 11:46:33AM +0000, Blue Swirl wrote:
>> Change checkpatch.pl for QEMU use:
>>  - Root directory detection
>>  - Forbid tabs
>>  - Indent at 4 spaces
>>  - Allow typedefs
>>  - Enforce brace use even for single statement blocks
>>  - Don't suggest nonexistent cleanup tools

BTW, I checked the last 99 commits with the script. The only real
problems I saw were that the script complains that CPP #if statements
are missing braces and the tabs check fires also for old code.

for i in '' 1 2 3 4 5 6 7 8 9; do for j in 0 1 2 3 4 5 6 7 8 9; do
echo $i$j; git show HEAD~$i$j >a; clear;cat a; perl
checkpatch.pl-0.31+QEMU --root=. a; read foo; done; done

> Could you separate out the coding style changes, please?
>
> Otherwise, it's just 2900 lines added and 2900 lines removed.

Does git-am understand patches in -M style? Is there a config option?

Here's the patch with rename detection on for easier commenting:
                next if ($realfile !~ /\.(h|c)$/);

@@ -1746,7 +1728,7 @@ sub process {

                        #print "line<$line> prevline<$prevline>
indent<$indent> sindent<$sindent> check<$check>
continuation<$continuation> s<$s> cond_lines<$cond_lines>
stat_real<$stat_real> stat<$stat>\n";

-                       if ($check && (($sindent % 8) != 0 ||
+                       if ($check && (($sindent % 4) != 0 ||
                            ($sindent <= $indent && $s ne ''))) {
                                WARN("suspect code indent for
conditional statements ($indent, $sindent)\n" . $herecurr .
"$stat_real\n");
                        }
@@ -1869,16 +1851,6 @@ sub process {
                                $herecurr);
                }

-# 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/) {
-                       WARN("do not add new typedefs\n" . $herecurr);
-               }
-
 # * goes on variable not on type
                # (char*[ const])
                if ($line =~ m{\($NonptrType(\s*(?:$Modifier\b\s*|\*\s*)+)\)}) {
@@ -2164,13 +2136,13 @@ sub process {
                                            ($op eq '>' &&
                                             $ca =~ /<\S+\@\S+$/))
                                        {
-                                               $ok = 1;
+                                               $ok = 1;
                                        }

                                        # Ignore ?:
                                        if (($opv eq ':O' && $ca =~ /\?$/) ||
                                            ($op eq '?' && $cc =~ /^:/)) {
-                                               $ok = 1;
+                                               $ok = 1;
                                        }

                                        if ($ok == 0) {
@@ -2514,13 +2486,13 @@ sub process {
                        WARN("vmlinux.lds.h needs VMLINUX_SYMBOL()
around C-visible symbols\n" . $herecurr);
                }

-# check for redundant bracing round if etc
+# check for missing bracing round if etc
                if ($line =~ /(^.*)\bif\b/ && $1 !~ /else\s*$/) {
                        my ($level, $endln, @chunks) =
                                ctx_statement_full($linenr, $realcnt, 1);
                        #print "chunks<$#chunks> linenr<$linenr>
endln<$endln> level<$level>\n";
                        #print "APW: <<$chunks[1][0]>><<$chunks[1][1]>>\n";
-                       if ($#chunks > 0 && $level == 0) {
+                       if ($#chunks >= 0 && $level == 0) {
                                my $allowed = 0;
                                my $seen = 0;
                                my $herectx = $here . "\n";
@@ -2558,8 +2530,8 @@ sub process {
                                                $allowed = 1;
                                        }
                                }
-                               if ($seen && !$allowed) {
-                                       WARN("braces {} are not
necessary for any arm of this statement\n" . $herectx);
+                               if (!$seen) {
+                                       WARN("braces {} are necessary
for all arms of this statement\n" . $herectx);
                                }
                        }
                }
@@ -2605,7 +2577,7 @@ sub process {
                                        $allowed = 1;
                                }
                        }
-                       if ($level == 0 && $block =~ /^\s*\{/ && !$allowed) {
+                       if ($level == 0 && !$block =~ /^\s*\{/ && !$allowed) {
                                my $herectx = $here . "\n";;
                                my $cnt = statement_rawlines($block);

@@ -2613,7 +2585,7 @@ sub process {
                                        $herectx .= raw_line($linenr,
$n) . "\n";;
                                }

-                               WARN("braces {} are not necessary for
single statement blocks\n" . $herectx);
+                               WARN("braces {} are necessary even for
single statement blocks\n" . $herectx);
                        }
                }

@@ -2918,10 +2890,10 @@ sub process {
        if ($quiet == 0) {
                # If there were whitespace errors which cleanpatch can fix
                # then suggest that.
-               if ($rpt_cleaners) {
-                       print "NOTE: whitespace errors detected, you
may wish to use scripts/cleanpatch or\n";
-                       print "      scripts/cleanfile\n\n";
-               }
+#              if ($rpt_cleaners) {
+#                      print "NOTE: whitespace errors detected, you
may wish to use scripts/cleanpatch or\n";
+#                      print "      scripts/cleanfile\n\n";
+#              }
        }

        if ($clean == 1 && $quiet == 0) {
Jan Kiszka - Jan. 9, 2011, 3:54 p.m.
Am 09.01.2011 14:24, Blue Swirl wrote:
> On Sun, Jan 9, 2011 at 1:01 PM, Stuart Brady <sdb@zubnet.me.uk> wrote:
>> On Sun, Jan 09, 2011 at 11:46:33AM +0000, Blue Swirl wrote:
>>> Change checkpatch.pl for QEMU use:
>>>  - Root directory detection
>>>  - Forbid tabs
>>>  - Indent at 4 spaces
>>>  - Allow typedefs
>>>  - Enforce brace use even for single statement blocks
>>>  - Don't suggest nonexistent cleanup tools
> 
> BTW, I checked the last 99 commits with the script. The only real
> problems I saw were that the script complains that CPP #if statements
> are missing braces and the tabs check fires also for old code.

I bet that's fixable with a few additional checks. :)

> 
> for i in '' 1 2 3 4 5 6 7 8 9; do for j in 0 1 2 3 4 5 6 7 8 9; do
> echo $i$j; git show HEAD~$i$j >a; clear;cat a; perl
> checkpatch.pl-0.31+QEMU --root=. a; read foo; done; done
> 
>> Could you separate out the coding style changes, please?
>>
>> Otherwise, it's just 2900 lines added and 2900 lines removed.
> 
> Does git-am understand patches in -M style? Is there a config option?

Would be surprising if not.

> 
> Here's the patch with rename detection on for easier commenting:

Line-wrapped unfortunately.

> diff --git a/checkpatch.pl-0.31 b/checkpatch.pl-0.31+QEMU
> similarity index 97%
> rename from checkpatch.pl-0.31
> rename to checkpatch.pl-0.31+QEMU

I prefer to track the origin in git, not in the file name. For
convenience, we need executable(!) checkpath.pl, at least a symbolic
link to it.

Otherwise, kudos for picking this up!

Jan

Patch

diff --git a/checkpatch.pl-0.31 b/checkpatch.pl-0.31+QEMU
similarity index 97%
rename from checkpatch.pl-0.31
rename to checkpatch.pl-0.31+QEMU
index cd51a65..8541916 100644
--- a/checkpatch.pl-0.31
+++ b/checkpatch.pl-0.31+QEMU
@@ -328,9 +328,9 @@  sub top_of_kernel_tree {
        my ($root) = @_;

        my @tree_check = (
-               "COPYING", "CREDITS", "Kbuild", "MAINTAINERS", "Makefile",
-               "README", "Documentation", "arch", "include", "drivers",
-               "fs", "init", "ipc", "kernel", "lib", "scripts",
+               "COPYING", "MAINTAINERS", "Makefile",
+               "README", "docs", "VERSION",
+               "vl.c"
        );

        foreach my $check (@tree_check) {
@@ -1494,31 +1494,13 @@  sub process {
 # check we are in a valid source file C or perl if not then ignore this hunk
                next if ($realfile !~ /\.(h|c|pl)$/);

-# at the beginning of a line any tabs must come first and anything
-# more than 8 must use tabs.
-               if ($rawline =~ /^\+\s* \t\s*\S/ ||
-                   $rawline =~ /^\+\s*        \s*/) {
+# in QEMU, no tabs are allowed
+               if ($rawline =~ /\t/) {
                        my $herevet = "$here\n" . cat_vet($rawline) . "\n";
-                       ERROR("code indent should use tabs where
possible\n" . $herevet);
+                       ERROR("code indent should never use tabs\n" . $herevet);
                        $rpt_cleaners = 1;
                }

-# check for space before tabs.
-               if ($rawline =~ /^\+/ && $rawline =~ / \t/) {
-                       my $herevet = "$here\n" . cat_vet($rawline) . "\n";
-                       WARN("please, no space before tabs\n" . $herevet);
-               }
-
-# check for spaces at the beginning of a line.
-# Exceptions:
-#  1) within comments
-#  2) indented preprocessor commands
-#  3) hanging labels
-               if ($rawline =~ /^\+ / && $line !~ /\+ *(?:$;|#|$Ident:)/)  {
-                       my $herevet = "$here\n" . cat_vet($rawline) . "\n";
-                       WARN("please, no spaces at the start of a
line\n" . $herevet);
-               }
-
 # check we are in a valid C source file if not then ignore this hunk