LinuxLists.cc - [PATCH] 0/3 checkpatch updates, new checkfiles script

2007-10-05 17:11:35

Subject: [PATCH] 0/3 checkpatch updates, new checkfiles script

This series of patches adds a -t option to checkpatch, so it can print terse
messages one per line, in a format compatible with g/cc
(filename:linenumber:message). This format can be parsed by various tools
and editors that can help show the errors in one window and the offending
file+line in another window.

This patch series also introduces a new small shell script wrapper, called
scripts/checkfiles, that checks the compliance of source files (not in diff
format); the script can operate on any mix of files and directories
(recursively checking). For example, to check the entire Linux kernel, run:

$ ./scripts/checkfiles .

So, I ran the above script and it found nearly 1.5 million reported
warnings/errors, with drivers being the largest abuser, not surprisingly.
Here's the sorted breakdown per top-level subsystem:

32 usr
162 init
266 block
293 Documentation
471 ipc
819 mm
1115 security
1915 scripts
1948 kernel
4569 lib
4793 crypto
13851 net
80025 sound
86484 include
115789 arch
116623 fs
1035158 drivers
-------+-------
1464313 TOTAL

Any volunteers? :-)

Erez Zadok (3):
CHECKPATCH: update usage string for checkpatch.pl
CHECKPATCH: add terse output option to checkpatch.pl
CHECKFILES: new small shell script to check multiple source files

checkfiles | 34 ++++++++++++++++++++++++++++++++++
checkpatch.pl | 21 +++++++++++++++++----
2 files changed, 51 insertions(+), 4 deletions(-)

---
Erez Zadok
[email protected]

2007-10-05 16:56:28

by Erez Zadok

[permalink] [raw]

Subject: [PATCH 3/3] CHECKFILES: new small shell script to check multiple source files

Examples:
./scripts/checkfiles fs/foo/bar.c
./scripts/checkfiles fs/foo/*.c
./scripts/checkfiles fs/foo # check all sources under fs/foo
./scripts/checkfiles . # check entire kernel

Signed-off-by: Erez Zadok <[email protected]>
---
scripts/checkfiles | 34 ++++++++++++++++++++++++++++++++++
1 files changed, 34 insertions(+), 0 deletions(-)
create mode 100644 scripts/checkfiles

diff --git a/scripts/checkfiles b/scripts/checkfiles
new file mode 100644
index 0000000..bd5d3f0
--- /dev/null
+++ b/scripts/checkfiles
@@ -0,0 +1,34 @@
+#!/bin/sh
+# (c) 2007, Erez Zadok <[email protected]> (initial version)
+# Licensed under the terms of the GNU GPL License version 2
+#
+# Check source files for compliance with coding standards, using terse
+# output in the style that g/cc produces. This output can be easily parsed
+# within text editors (e.g., emacs/vim) which can produce a split text
+# screen showing in one screen the error message, and in another screen the
+# corresponding source file, with the cursor placed on the offending line.
+# See for example the documentation for Emacs's "next-error" command, often
+# bound to M-x ` (ESC x back-tick).
+
+# Usage: checkfiles file [files...]
+# if "file" is a directory, will check all *.[hc] files recursively
+
+# check usage
+usage() {
+ echo "Usage: checkfiles file [files...]"
+ echo "(if \"file\" is a directory, check recursively for all C sources/headers)"
+ exit 1
+}
+if test -z "$1" ; then
+ usage
+fi
+if ! test -f scripts/checkpatch.pl ; then
+ echo "checkfiles: must run from top level source tree"
+ exit 1
+fi
+
+# check coding-style compliance of each source file found, using terse output
+find "$@" -type f -name '*.[hc]' | \
+while read f ; do
+ diff -u /dev/null $f | perl scripts/checkpatch.pl -t -
+done
--
1.5.2.2

2007-10-05 16:56:43

by Erez Zadok

[permalink] [raw]

Subject: [PATCH 2/3] CHECKPATCH: add terse output option to checkpatch.pl

Such terse output complies with g/cc and looks like

file_name:line_number:error_message

This output can be easily parsed within text editors (e.g., emacs/vim) that
can produce a split text screen showing in one screen the error message, and
in another screen the corresponding source file, with the cursor placed on
the offending line.

Signed-off-by: Erez Zadok <[email protected]>
---
scripts/checkpatch.pl | 20 ++++++++++++++++----
1 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index ecbb030..f8bd630 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -18,12 +18,14 @@ my $tree = 1;
my $chk_signoff = 1;
my $chk_patch = 1;
my $tst_type = 0;
+my $terse = 0; # terse ouput: report using g/cc error style
GetOptions(
'q|quiet' => \$quiet,
'tree!' => \$tree,
'signoff!' => \$chk_signoff,
'patch!' => \$chk_patch,
'test-type!' => \$tst_type,
+ 't|terse' => \$terse,
) or exit;

my $exit = 0;
@@ -34,6 +36,7 @@ if ($#ARGV < 0) {
print "options: -q => quiet\n";
print " --no-tree => run without a kernel tree\n";
print " --no-signoff => don't check for signed-off-by\n";
+ print " -t => report errors using terse cc-style output (implies -q)\n";
exit(1);
}

@@ -267,7 +270,16 @@ sub cat_vet {

my @report = ();
sub report {
- push(@report, $_[0]);
+ if (!$terse) {
+ push(@report, $_[0]);
+ return;
+ }
+ # if terse output, extract filename, linenumber, and short message;
+ # format them as a new one-line message and push onto report
+ my($msg, $location, @rest) = split(/\n/, $_[0]);
+ @rest = split(/: /, $location);
+ my($newreport) = sprintf("%s%s\n", $rest[2], $msg);
+ push(@report, $newreport);
}
sub report_dump {
@report;
@@ -1097,17 +1109,17 @@ sub process {
if ($chk_patch && !$is_patch) {
ERROR("Does not appear to be a unified-diff format patch\n");
}
- if ($is_patch && $chk_signoff && $signoff == 0) {
+ if ($terse == 0 && $is_patch && $chk_signoff && $signoff == 0) {
ERROR("Missing Signed-off-by: line(s)\n");
}

if ($clean == 0 && ($chk_patch || $is_patch)) {
print report_dump();
}
- if ($clean == 1 && $quiet == 0) {
+ if ($terse == 0 && $clean == 1 && $quiet == 0) {
print "Your patch has no obvious style problems and is ready for submission.\n"
}
- if ($clean == 0 && $quiet == 0) {
+ if ($terse == 0 && $clean == 0 && $quiet == 0) {
print "Your patch has style problems, please review. If any of these errors\n";
print "are false positives report them to the maintainer, see\n";
print "CHECKPATCH in MAINTAINERS.\n";
--
1.5.2.2

2007-10-05 16:59:37

by Erez Zadok

[permalink] [raw]

Subject: [PATCH 1/3] CHECKPATCH: update usage string for checkpatch.pl

Signed-off-by: Erez Zadok <[email protected]>
---
scripts/checkpatch.pl | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index dae7d30..ecbb030 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -33,6 +33,7 @@ if ($#ARGV < 0) {
print "version: $V\n";
print "options: -q => quiet\n";
print " --no-tree => run without a kernel tree\n";
+ print " --no-signoff => don't check for signed-off-by\n";
exit(1);
}

--
1.5.2.2

2007-10-06 11:14:07

by Ingo Molnar

[permalink] [raw]

Subject: Re: [PATCH] 0/3 checkpatch updates, new checkfiles script

* Erez Zadok <[email protected]> wrote:

> So, I ran the above script and it found nearly 1.5 million reported
> warnings/errors, with drivers being the largest abuser, not
> surprisingly. [...]

have you tried that with the latest version too:

http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-next

it outputs far fewer false positives.

Ingo

2007-10-07 19:07:46

by Erez Zadok

[permalink] [raw]

Subject: Re: [PATCH] 0/3 checkpatch updates, new checkfiles script

In message <[email protected]>, Ingo Molnar writes:
>
> * Erez Zadok <[email protected]> wrote:
>
> > So, I ran the above script and it found nearly 1.5 million reported
> > warnings/errors, with drivers being the largest abuser, not
> > surprisingly. [...]
>
> have you tried that with the latest version too:
>
> http://www.kernel.org/pub/linux/kernel/people/apw/checkpatch/checkpatch.pl-next
>
> it outputs far fewer false positives.
>
> Ingo

Andy, Ingo,

I tried the new checkpatch.pl on 2.6.23-rc9:

$ ./scripts/checkpatch.pl -q --file --emacs fs/namei.c

and got many perl warnings such as:

Use of uninitialized value in concatenation (.) or string at ./scripts/checkpatch.pl line 455.

followed by the usual verbose error message instead of one-per-line as I
assume the --emacs option is supposed to produce:

:2823: WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
#2823: FILE: namei.c:2820:
+EXPORT_SYMBOL(vfs_mkdir);

BTW, calling the option --emacs is a bit too restrictive. Emacs didn't
invent the format of "filename:linenumeber:message". C compilers had it
before. Even "grep -n *" had it before. That's why I think calling it a
"terse output" option may be more accurate.

The following small patch to checkpath.pl-next seems to fix the perl
warnings, but it still outputs the long error messages along with the
shorter one-liners.

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index bdc493e..bbc4825 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -85,7 +85,7 @@ for my $filename (@ARGV) {
push(@rawlines, $_);
}
close(FILE);
- if (!process($ARGV, @rawlines)) {
+ if (!process($filename, @rawlines)) {
$exit = 1;
}
@rawlines = ();
@@ -452,7 +452,7 @@ sub process {

my $rawline = $line;

- $prefix = "$ARGV:$linenr: " if ($emacs);
+ $prefix = "$filename:$linenr: " if ($emacs);

#extract the filename as it passes
if ($line=~/^\+\+\+\s+(\S+)/) {

Cheers,
Erez.

2007-10-11 13:48:09

by Andy Whitcroft

[permalink] [raw]

Subject: Re: [PATCH] 0/3 checkpatch updates, new checkfiles script

On Sun, Oct 07, 2007 at 03:05:47PM -0400, Erez Zadok wrote:
>
> and got many perl warnings such as:
>
> Use of uninitialized value in concatenation (.) or string at ./scripts/checkpatch.pl line 455.

Yes, this support seems to be wholy broken, as a non emacs user I had
failed to test it correctly as I added the --file option. Bad Andy.

In testing it I note we are emitting wholy the wrong line number, and
the filename was off as you fixed up in your patch.

> followed by the usual verbose error message instead of one-per-line as I
> assume the --emacs option is supposed to produce:
>
> :2823: WARNING: EXPORT_SYMBOL(foo); should immediately follow its function/variable
> #2823: FILE: namei.c:2820:
> +EXPORT_SYMBOL(vfs_mkdir);
>
> BTW, calling the option --emacs is a bit too restrictive. Emacs didn't
> invent the format of "filename:linenumeber:message". C compilers had it
> before. Even "grep -n *" had it before. That's why I think calling it a
> "terse output" option may be more accurate.
>
> The following small patch to checkpath.pl-next seems to fix the perl
> warnings, but it still outputs the long error messages along with the
> shorter one-liners.

As I understand things this is called emacs mode because the emacs
buffer mode expects the filename:line:message format with the long error
to follow. It seems pretty emacs specific. So for now I'll leave it
named that. If people convince me its --compiler-format or something we
can add that as an alias later.

I have hopefully sorted the main problems with it and will push out an
update for testing.

-apw

2007-10-13 18:56:32

by Erez Zadok

[permalink] [raw]