[Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Michael Roth]

Was playing around with Stefan's git hook for checkpatch.pl:

http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

which seems really useful butter-finger coders such as myself. But some of warnings/errors that have carried over from the kernel have made this approach somewhat prohibitive for QEMU code.

These patches add a flag that let's checkpatch.pl exit successfully when we only have warnings, while still printing them and encouraging users to fix them.

We also make C99 //comments a warning instead of an error, since they don't actually violate QEMU's coding guidelines.

[Qemu-devel] [PATCH 1/2] checkpatch.pl: add --no-fail-on-warn option

[Michael Roth]

checkpatch.pl complains about some things that are not strictly against
QEMU's coding style guidelines. It's good to print these, but we
shouldn't force a fail on these as it makes it difficult to automate
checkpatch.pl runs. If we're supposed to fail on these cases, they
should be handled as errors rather than warnings.

For now, however, just add a flag that enables this behavior.

Any functionality triggered by $clean = 0 is still handled as it was
previously.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 scripts/checkpatch.pl |   11 ++++++++++-
 1 files changed, 10 insertions(+), 1 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 075b614..770d534 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -28,6 +28,7 @@ my $mailback = 0;
 my $summary_file = 0;
 my $root;
 my %debug;
+my $no_fail_on_warn = 0;
 my $help = 0;
 
 sub help {
@@ -55,6 +56,7 @@ Options:
                              is all off)
   --test-only=WORD           report only warnings/errors containing WORD
                              literally
+  --no-fail-on-warn          print warnings, but don't fail on them
   -h, --help, --version      display this help and exit
 
 When FILE is - read standard input.
@@ -80,6 +82,7 @@ GetOptions(
 
 	'debug=s'	=> \%debug,
 	'test-only=s'	=> \$tst_only,
+	'no-fail-on-warn' => \$no_fail_on_warn,
 	'h|help'	=> \$help,
 	'version'	=> \$help
 ) or help(1);
@@ -1104,18 +1107,23 @@ sub report_dump {
 sub ERROR {
 	if (report("ERROR: $_[0]\n")) {
 		our $clean = 0;
+		our $passable = 0;
 		our $cnt_error++;
 	}
 }
 sub WARN {
 	if (report("WARNING: $_[0]\n")) {
 		our $clean = 0;
+		if ($no_fail_on_warn == 0) {
+			our $passable = 0;
+		}
 		our $cnt_warn++;
 	}
 }
 sub CHK {
 	if ($check && report("CHECK: $_[0]\n")) {
 		our $clean = 0;
+		our $passable = 0;
 		our $cnt_chk++;
 	}
 }
@@ -1162,6 +1170,7 @@ sub process {
 	my $stashindent=0;
 
 	our $clean = 1;
+	our $passable = 1;
 	my $signoff = 0;
 	my $is_patch = 0;
 
@@ -2906,5 +2915,5 @@ sub process {
 		print "CHECKPATCH in MAINTAINERS.\n";
 	}
 
-	return $clean;
+	return $passable;
 }
-- 
1.7.0.4

[Qemu-devel] [PATCH 2/2] checkpatch.pl: make C99 comments a warning, not error

[Michael Roth]

C99 comments are pretty heavilly used in QEMU, and don't violate
anything mentioned in HACKING/CODING_STYLE. Make them warnings instead.

Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
---
 scripts/checkpatch.pl |    2 +-
 test.c                |    5 +++++
 2 files changed, 6 insertions(+), 1 deletions(-)
 create mode 100644 test.c

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 770d534..2aab4e9 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1799,7 +1799,7 @@ sub process {
 
 # no C99 // comments
 		if ($line =~ m{//}) {
-			ERROR("do not use C99 // comments\n" . $herecurr);
+			WARN("do not use C99 // comments\n" . $herecurr);
 		}
 		# Remove C99 comments.
 		$line =~ s@//.*@@;
diff --git a/test.c b/test.c
new file mode 100644
index 0000000..fe3b163
--- /dev/null
+++ b/test.c
@@ -0,0 +1,5 @@
+//stuff
+static int blah(void)
+{
+    return 1;
+}
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 2/2] checkpatch.pl: make C99 comments a warning, not error

[Michael Roth]

On 04/01/2011 10:20 AM, Michael Roth wrote:
> C99 comments are pretty heavilly used in QEMU, and don't violate
> anything mentioned in HACKING/CODING_STYLE. Make them warnings instead.
>
> Signed-off-by: Michael Roth<mdroth@linux.vnet.ibm.com>
> ---
>   scripts/checkpatch.pl |    2 +-
>   test.c                |    5 +++++
>   2 files changed, 6 insertions(+), 1 deletions(-)
>   create mode 100644 test.c
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 770d534..2aab4e9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1799,7 +1799,7 @@ sub process {
>
>   # no C99 // comments
>   		if ($line =~ m{//}) {
> -			ERROR("do not use C99 // comments\n" . $herecurr);
> +			WARN("do not use C99 // comments\n" . $herecurr);
>   		}
>   		# Remove C99 comments.
>   		$line =~ s@//.*@@;
> diff --git a/test.c b/test.c
> new file mode 100644
> index 0000000..fe3b163
> --- /dev/null
> +++ b/test.c
> @@ -0,0 +1,5 @@
> +//stuff
> +static int blah(void)
> +{
> +    return 1;
> +}

Ugh, please ignore this test.c addition. Let me know if I should resend

Re: [Qemu-devel] [PATCH 2/2] checkpatch.pl: make C99 comments a warning, not error

[Stefan Hajnoczi]

On Fri, Apr 1, 2011 at 4:20 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> C99 comments are pretty heavilly used in QEMU, and don't violate
> anything mentioned in HACKING/CODING_STYLE. Make them warnings instead.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  scripts/checkpatch.pl |    2 +-
>  test.c                |    5 +++++
>  2 files changed, 6 insertions(+), 1 deletions(-)
>  create mode 100644 test.c

Reviewed-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

Re: [Qemu-devel] [PATCH 2/2] checkpatch.pl: make C99 comments a warning, not error

[Blue Swirl]

On Fri, Apr 1, 2011 at 6:20 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> C99 comments are pretty heavilly used in QEMU, and don't violate
> anything mentioned in HACKING/CODING_STYLE. Make them warnings instead.
>
> Signed-off-by: Michael Roth <mdroth@linux.vnet.ibm.com>
> ---
>  scripts/checkpatch.pl |    2 +-
>  test.c                |    5 +++++
>  2 files changed, 6 insertions(+), 1 deletions(-)
>  create mode 100644 test.c
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 770d534..2aab4e9 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -1799,7 +1799,7 @@ sub process {
>
>  # no C99 // comments
>                if ($line =~ m{//}) {
> -                       ERROR("do not use C99 // comments\n" . $herecurr);
> +                       WARN("do not use C99 // comments\n" . $herecurr);

I think the only reasonable use for C99 comments is
//#define DEBUG_xyz
for quickly enabling debugging printfs.

But even this pattern should be replaced by tracepoints, since they
are much better. So I'd prefer to keep the ERROR.

Re: [Qemu-devel] [PATCH 2/2] checkpatch.pl: make C99 comments a warning, not error

[Stefan Hajnoczi]

On Fri, Apr 1, 2011 at 7:46 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> I think the only reasonable use for C99 comments is
> //#define DEBUG_xyz
> for quickly enabling debugging printfs.
>
> But even this pattern should be replaced by tracepoints, since they
> are much better. So I'd prefer to keep the ERROR.

Please update CODING_STYLE.

Stefan

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Peter Maydell]

On 1 April 2011 16:20, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> We also make C99 //comments a warning instead of an error, since they
> don't actually violate QEMU's coding guidelines.

We should either update the guidelines or fix the script...

-- PMM

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Stefan Hajnoczi]

On Fri, Apr 1, 2011 at 4:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 1 April 2011 16:20, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>> We also make C99 //comments a warning instead of an error, since they
>> don't actually violate QEMU's coding guidelines.
>
> We should either update the guidelines or fix the script...

There are a whole bunch of // in the codebase.  I prefer /* */ but as
it stands I think // should not even raise a warning.

Stefan

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Peter Maydell]

On 1 April 2011 16:59, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Fri, Apr 1, 2011 at 4:52 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 1 April 2011 16:20, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
>>> We also make C99 //comments a warning instead of an error, since they
>>> don't actually violate QEMU's coding guidelines.
>>
>> We should either update the guidelines or fix the script...
>
> There are a whole bunch of // in the codebase.  I prefer /* */ but as
> it stands I think // should not even raise a warning.

I don't care much either, really. I just don't think we should be
introducing random coding standards rules by the back door because
checkpatch happens to complain about them.

-- PMM

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Michael Roth]

On 04/01/2011 11:16 AM, Peter Maydell wrote:
> On 1 April 2011 16:59, Stefan Hajnoczi<stefanha@gmail.com>  wrote:
>> On Fri, Apr 1, 2011 at 4:52 PM, Peter Maydell<peter.maydell@linaro.org>  wrote:
>>> On 1 April 2011 16:20, Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>>>> We also make C99 //comments a warning instead of an error, since they
>>>> don't actually violate QEMU's coding guidelines.
>>>
>>> We should either update the guidelines or fix the script...
>>
>> There are a whole bunch of // in the codebase.  I prefer /* */ but as
>> it stands I think // should not even raise a warning.
>
> I don't care much either, really. I just don't think we should be
> introducing random coding standards rules by the back door because
> checkpatch happens to complain about them.

Whether or not // comments should also be a warning...I'm not sure. It 
seems like a reasonable "suggestion" to make though, since mixed comment 
styles makes code look nastier. I could also go either way though...

But there *are* some warnings that make sense to complain about without 
saying "you can't do this", like extern's in .c files: some cases are 
exceptional.

I'd prefer to only document "strict" guidelines, and treat checkpatch.pl 
warnings ("suggestions") as an extra "reward" you get for taking the 
time to run it.

>
> -- PMM

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Peter Maydell]

On 1 April 2011 17:55, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> But there *are* some warnings that make sense to complain about without
> saying "you can't do this", like extern's in .c files: some cases are
> exceptional.

I'd treat everything checkpatch says as a warning anyway, because it gets
confused by things like macros often enough that you can't guarantee
that everything it thinks is an error is truly an error.

-- PMM

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Michael Roth]

On 04/01/2011 11:58 AM, Peter Maydell wrote:
> On 1 April 2011 17:55, Michael Roth<mdroth@linux.vnet.ibm.com>  wrote:
>> But there *are* some warnings that make sense to complain about without
>> saying "you can't do this", like extern's in .c files: some cases are
>> exceptional.
>
> I'd treat everything checkpatch says as a warning anyway, because it gets
> confused by things like macros often enough that you can't guarantee
> that everything it thinks is an error is truly an error.

Well, we always have the option of ignoring the return code and just 
looking at the output. Would be nice to be able to automate the check 
somewhat reliably though, and just override the false positives when 
they pop up.

>
> -- PMM

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Stefan Hajnoczi]

On Fri, Apr 01, 2011 at 11:55:39AM -0500, Michael Roth wrote:
> I'd prefer to only document "strict" guidelines, and treat
> checkpatch.pl warnings ("suggestions") as an extra "reward" you get
> for taking the time to run it.

I don't want to be punished for running checkpatch.pl like I'm supposed
to while those who don't can get away with more.

A --pedantic mode would be fine although probably no one besides the
author would use it :).

The stuff that gets reported by the default invocation needs to matter,
otherwise checkpatch.pl isn't useful and people will bypass it.

Stefan

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Michael Roth]

On 04/01/2011 12:01 PM, Stefan Hajnoczi wrote:
> On Fri, Apr 01, 2011 at 11:55:39AM -0500, Michael Roth wrote:
>> I'd prefer to only document "strict" guidelines, and treat
>> checkpatch.pl warnings ("suggestions") as an extra "reward" you get
>> for taking the time to run it.
>
> I don't want to be punished for running checkpatch.pl like I'm supposed
> to while those who don't can get away with more.

You're not! These are extra morsels of goodness :)

>
> A --pedantic mode would be fine although probably no one besides the
> author would use it :).

True :) But you're right, this is probably the better approach. How bout:

--warnings: print coding style warnings in addition to errors, and exit 
failure if encountered

Then default to suppressing warning statements, and --no-fail-on-warn 
behavior.

>
> The stuff that gets reported by the default invocation needs to matter,
> otherwise checkpatch.pl isn't useful and people will bypass it.
>
> Stefan

Re: [Qemu-devel] checkpatch.pl: warn on C99 comments, but don't fail

[Stefan Hajnoczi]

On Fri, Apr 1, 2011 at 6:25 PM, Michael Roth <mdroth@linux.vnet.ibm.com> wrote:
> On 04/01/2011 12:01 PM, Stefan Hajnoczi wrote:
>>
>> On Fri, Apr 01, 2011 at 11:55:39AM -0500, Michael Roth wrote:
>>>
>>> I'd prefer to only document "strict" guidelines, and treat
>>> checkpatch.pl warnings ("suggestions") as an extra "reward" you get
>>> for taking the time to run it.
>>
>> I don't want to be punished for running checkpatch.pl like I'm supposed
>> to while those who don't can get away with more.
>
> You're not! These are extra morsels of goodness :)
>
>>
>> A --pedantic mode would be fine although probably no one besides the
>> author would use it :).
>
> True :) But you're right, this is probably the better approach. How bout:
>
> --warnings: print coding style warnings in addition to errors, and exit
> failure if encountered
>
> Then default to suppressing warning statements, and --no-fail-on-warn
> behavior.

Sounds good to me :).

Stefan