diff mbox

[2/3] checkpatch: bump most warnings to errors

Message ID 1470757664-25161-3-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Aug. 9, 2016, 3:47 p.m. UTC 
This only leaves a warning-level message for extra-long lines, which
are relatively common and cause patchew to send email that will likely
be ignored.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 scripts/checkpatch.pl | 66 +++++++++++++++++++++++++--------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

Comments

Markus Armbruster Aug. 10, 2016, 6:53 a.m. UTC | #1 
Paolo Bonzini <pbonzini@redhat.com> writes:

> This only leaves a warning-level message for extra-long lines, which
> are relatively common and cause patchew to send email that will likely
> be ignored.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Are we ready to give up on illegibly long lines?

What happened to "[PATCH 2/4] CODING_STYLE, checkpatch: update line
length rules"?
Paolo Bonzini Aug. 10, 2016, 7:08 a.m. UTC | #2 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> > This only leaves a warning-level message for extra-long lines, which
> > are relatively common and cause patchew to send email that will likely
> > be ignored.
> >
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Are we ready to give up on illegibly long lines?

We have other levels of code review than checkpatch.  80 chars can be
illegibly short in some circumstances where 83 or 84 are enough.

Paolo

> What happened to "[PATCH 2/4] CODING_STYLE, checkpatch: update line
> length rules"?
Cornelia Huck Aug. 10, 2016, 7:46 a.m. UTC | #3 
On Tue,  9 Aug 2016 17:47:43 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:

> This only leaves a warning-level message for extra-long lines, which
> are relatively common and cause patchew to send email that will likely
> be ignored.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  scripts/checkpatch.pl | 66 +++++++++++++++++++++++++--------------------------
>  1 file changed, 33 insertions(+), 33 deletions(-)
> 

>  # no volatiles please
>  		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
>  		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
> -			WARN("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
> +			ERROR("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);

It's a bit weird to have this refer to a Linux file :)
Markus Armbruster Aug. 10, 2016, 7:48 a.m. UTC | #4 
Paolo Bonzini <pbonzini@redhat.com> writes:

>> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 
>> > This only leaves a warning-level message for extra-long lines, which
>> > are relatively common and cause patchew to send email that will likely
>> > be ignored.
>> >
>> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> 
>> Are we ready to give up on illegibly long lines?
>
> We have other levels of code review than checkpatch.  80 chars can be
> illegibly short in some circumstances where 83 or 84 are enough.

Isn't that addressed neatly in your patch?  It has a soft and a hard
limit.  Exceeding the hard limit is an error, exceeding the soft limit
is a warning.  I rather liked that.  If I remember correctly, the only
disagreements were about the value of the soft limit.

> Paolo
>
>> What happened to "[PATCH 2/4] CODING_STYLE, checkpatch: update line
>> length rules"?
Cornelia Huck Aug. 10, 2016, 7:48 a.m. UTC | #5 
On Wed, 10 Aug 2016 03:08:33 -0400 (EDT)
Paolo Bonzini <pbonzini@redhat.com> wrote:

> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> > > This only leaves a warning-level message for extra-long lines, which
> > > are relatively common and cause patchew to send email that will likely
> > > be ignored.
> > >
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > Are we ready to give up on illegibly long lines?
> 
> We have other levels of code review than checkpatch.  80 chars can be
> illegibly short in some circumstances where 83 or 84 are enough.

We could leave the WARN at 80 chars and add an error at 120 chars or so.
Paolo Bonzini Aug. 10, 2016, 7:58 a.m. UTC | #6 
----- Original Message -----
> From: "Cornelia Huck" <cornelia.huck@de.ibm.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: qemu-devel@nongnu.org, thuth@redhat.com, famz@redhat.com, armbru@redhat.com
> Sent: Wednesday, August 10, 2016 9:46:14 AM
> Subject: Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors
> 
> On Tue,  9 Aug 2016 17:47:43 +0200
> Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > This only leaves a warning-level message for extra-long lines, which
> > are relatively common and cause patchew to send email that will likely
> > be ignored.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  scripts/checkpatch.pl | 66
> >  +++++++++++++++++++++++++--------------------------
> >  1 file changed, 33 insertions(+), 33 deletions(-)
> > 
> 
> >  # no volatiles please
> >  		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
> >  		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
> > -			WARN("Use of volatile is usually wrong: see
> > Documentation/volatile-considered-harmful.txt\n" . $herecurr);
> > +			ERROR("Use of volatile is usually wrong: see
> > Documentation/volatile-considered-harmful.txt\n" . $herecurr);
> 
> It's a bit weird to have this refer to a Linux file :)

Right.  We should include the explanation in docs/atomics.txt and point
to that.

Paolo
Paolo Bonzini Aug. 10, 2016, 7:58 a.m. UTC | #7 
----- Original Message -----
> From: "Markus Armbruster" <armbru@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: thuth@redhat.com, famz@redhat.com, qemu-devel@nongnu.org
> Sent: Wednesday, August 10, 2016 9:48:24 AM
> Subject: Re: [Qemu-devel] [PATCH 2/3] checkpatch: bump most warnings to errors
> 
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
> >> Paolo Bonzini <pbonzini@redhat.com> writes:
> >> 
> >> > This only leaves a warning-level message for extra-long lines, which
> >> > are relatively common and cause patchew to send email that will likely
> >> > be ignored.
> >> >
> >> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> 
> >> Are we ready to give up on illegibly long lines?
> >
> > We have other levels of code review than checkpatch.  80 chars can be
> > illegibly short in some circumstances where 83 or 84 are enough.
> 
> Isn't that addressed neatly in your patch?  It has a soft and a hard
> limit.  Exceeding the hard limit is an error, exceeding the soft limit
> is a warning.  I rather liked that.  If I remember correctly, the only
> disagreements were about the value of the soft limit.

Yes, indeed.  I can respin the patch then.

Paolo
diff mbox

Patch

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 7ccf6a8..cea1997 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -1289,11 +1289,11 @@  sub process {
 			# This is a signoff, if ugly, so do not double report.
 			$signoff++;
 			if (!($line =~ /^\s*Signed-off-by:/)) {
-				WARN("Signed-off-by: is the preferred form\n" .
+				ERROR("Signed-off-by: is the preferred form\n" .
 					$herecurr);
 			}
 			if ($line =~ /^\s*signed-off-by:\S/i) {
-				WARN("space required after Signed-off-by:\n" .
+				ERROR("space required after Signed-off-by:\n" .
 					$herecurr);
 			}
 		}
@@ -1343,12 +1343,12 @@  sub process {
 
 # check for spaces before a quoted newline
 		if ($rawline =~ /^.*\".*\s\\n/) {
-			WARN("unnecessary whitespace before a quoted newline\n" . $herecurr);
+			ERROR("unnecessary whitespace before a quoted newline\n" . $herecurr);
 		}
 
 # check for adding lines without a newline.
 		if ($line =~ /^\+/ && defined $lines[$linenr] && $lines[$linenr] =~ /^\\ No newline at end of file/) {
-			WARN("adding a line without newline at end of file\n" . $herecurr);
+			ERROR("adding a line without newline at end of file\n" . $herecurr);
 		}
 
 # check we are in a valid source file; if not then tabs are allowed.
@@ -1368,7 +1368,7 @@  sub process {
 
 # check for RCS/CVS revision markers
 		if ($rawline =~ /^\+.*\$(Revision|Log|Id)(?:\$|)/) {
-			WARN("CVS style keyword markers, these will _not_ be updated\n". $herecurr);
+			ERROR("CVS style keyword markers, these will _not_ be updated\n". $herecurr);
 		}
 
 # Check for potential 'bare' types
@@ -1500,7 +1500,7 @@  sub process {
 			{
 				my ($nlength, $nindent) = line_stats($lines[$ctx_ln - 1]);
 				if ($nindent > $indent) {
-					WARN("trailing semicolon indicates no statements, indent implies otherwise\n" .
+					ERROR("trailing semicolon indicates no statements, indent implies otherwise\n" .
 						"$here\n$ctx\n$rawlines[$ctx_ln - 1]\n");
 				}
 			}
@@ -1588,7 +1588,7 @@  sub process {
 
 			if ($check && (($sindent % 4) != 0 ||
 			    ($sindent <= $indent && $s ne ''))) {
-				WARN("suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
+				ERROR("suspect code indent for conditional statements ($indent, $sindent)\n" . $herecurr . "$stat_real\n");
 			}
 		}
 
@@ -1766,7 +1766,7 @@  sub process {
 			} elsif ($ctx =~ /$Type$/) {
 
 			} else {
-				WARN("space prohibited between function name and open parenthesis '('\n" . $herecurr);
+				ERROR("space prohibited between function name and open parenthesis '('\n" . $herecurr);
 			}
 		}
 # Check operator spacing.
@@ -2005,7 +2005,7 @@  sub process {
 		if ($line =~ /^.\s*return\s*(E[A-Z]*)\s*;/) {
 			my $name = $1;
 			if ($name ne 'EOF' && $name ne 'ERROR') {
-				WARN("return of an errno should typically be -ve (return -$1)\n" . $herecurr);
+				ERROR("return of an errno should typically be -ve (return -$1)\n" . $herecurr);
 			}
 		}
 
@@ -2077,7 +2077,7 @@  sub process {
 				(?:\&\&|\|\||\)|\])
 			)/x)
 		{
-			WARN("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr);
+			ERROR("boolean test with hexadecimal, perhaps just 1 \& or \|?\n" . $herecurr);
 		}
 
 # if and else should not have general statements after it
@@ -2133,7 +2133,7 @@  sub process {
 
 #no spaces allowed after \ in define
 		if ($line=~/\#\s*define.*\\\s$/) {
-			WARN("Whitepspace after \\ makes next lines useless\n" . $herecurr);
+			ERROR("Whitespace after \\ makes next lines useless\n" . $herecurr);
 		}
 
 # multi-statement macros should be enclosed in a do while loop, grab the
@@ -2285,7 +2285,7 @@  sub process {
 					}
 				}
 				if ($seen != ($#chunks + 1)) {
-					WARN("braces {} are necessary for all arms of this statement\n" . $herectx);
+					ERROR("braces {} are necessary for all arms of this statement\n" . $herectx);
 				}
 			}
 		}
@@ -2353,19 +2353,19 @@  sub process {
 					$herectx .= raw_line($linenr, $n) . "\n";;
 				}
 
-				WARN("braces {} are necessary even for single statement blocks\n" . $herectx);
+				ERROR("braces {} are necessary even for single statement blocks\n" . $herectx);
 			}
 		}
 
 # no volatiles please
 		my $asm_volatile = qr{\b(__asm__|asm)\s+(__volatile__|volatile)\b};
 		if ($line =~ /\bvolatile\b/ && $line !~ /$asm_volatile/) {
-			WARN("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
+			ERROR("Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n" . $herecurr);
 		}
 
 # warn about #if 0
 		if ($line =~ /^.\s*\#\s*if\s+0\b/) {
-			WARN("if this code is redundant consider removing it\n" .
+			ERROR("if this code is redundant consider removing it\n" .
 				$herecurr);
 		}
 
@@ -2373,7 +2373,7 @@  sub process {
 		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
 			my $expr = $1;
 			if ($line =~ /\bg_free\(\Q$expr\E\);/) {
-				WARN("g_free(NULL) is safe this check is probably not required\n" . $hereprev);
+				ERROR("g_free(NULL) is safe this check is probably not required\n" . $hereprev);
 			}
 		}
 
@@ -2391,19 +2391,19 @@  sub process {
 # check for memory barriers without a comment.
 		if ($line =~ /\b(smp_mb|smp_rmb|smp_wmb|smp_read_barrier_depends)\(/) {
 			if (!ctx_has_comment($first_line, $linenr)) {
-				WARN("memory barrier without comment\n" . $herecurr);
+				ERROR("memory barrier without comment\n" . $herecurr);
 			}
 		}
 # check of hardware specific defines
 # we have e.g. CONFIG_LINUX and CONFIG_WIN32 for common cases
 # where they might be necessary.
 		if ($line =~ m@^.\s*\#\s*if.*\b__@) {
-			WARN("architecture specific defines should be avoided\n" .  $herecurr);
+			ERROR("architecture specific defines should be avoided\n" .  $herecurr);
 		}
 
 # Check that the storage class is at the beginning of a declaration
 		if ($line =~ /\b$Storage\b/ && $line !~ /^.\s*$Storage\b/) {
-			WARN("storage class should be at the beginning of the declaration\n" . $herecurr)
+			ERROR("storage class should be at the beginning of the declaration\n" . $herecurr)
 		}
 
 # check the location of the inline attribute, that it is between
@@ -2415,7 +2415,7 @@  sub process {
 
 # check for sizeof(&)
 		if ($line =~ /\bsizeof\s*\(\s*\&/) {
-			WARN("sizeof(& should be avoided\n" . $herecurr);
+			ERROR("sizeof(& should be avoided\n" . $herecurr);
 		}
 
 # check for new externs in .c files.
@@ -2432,40 +2432,40 @@  sub process {
 			if ($s =~ /^\s*;/ &&
 			    $function_name ne 'uninitialized_var')
 			{
-				WARN("externs should be avoided in .c files\n" .  $herecurr);
+				ERROR("externs should be avoided in .c files\n" .  $herecurr);
 			}
 
 			if ($paren_space =~ /\n/) {
-				WARN("arguments for function declarations should follow identifier\n" . $herecurr);
+				ERROR("arguments for function declarations should follow identifier\n" . $herecurr);
 			}
 
 		} elsif ($realfile =~ /\.c$/ && defined $stat &&
 		    $stat =~ /^.\s*extern\s+/)
 		{
-			WARN("externs should be avoided in .c files\n" .  $herecurr);
+			ERROR("externs should be avoided in .c files\n" .  $herecurr);
 		}
 
 # check for pointless casting of g_malloc return
 		if ($line =~ /\*\s*\)\s*g_(try)?(m|re)alloc(0?)(_n)?\b/) {
 			if ($2 == 'm') {
-				WARN("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr);
+				ERROR("unnecessary cast may hide bugs, use g_$1new$3 instead\n" . $herecurr);
 			} else {
-				WARN("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr);
+				ERROR("unnecessary cast may hide bugs, use g_$1renew$3 instead\n" . $herecurr);
 			}
 		}
 
 # check for gcc specific __FUNCTION__
 		if ($line =~ /__FUNCTION__/) {
-			WARN("__func__ should be used instead of gcc specific __FUNCTION__\n"  . $herecurr);
+			ERROR("__func__ should be used instead of gcc specific __FUNCTION__\n"  . $herecurr);
 		}
 
 # recommend qemu_strto* over strto* for numeric conversions
 		if ($line =~ /\b(strto[^kd].*?)\s*\(/) {
-			WARN("consider using qemu_$1 in preference to $1\n" . $herecurr);
+			ERROR("consider using qemu_$1 in preference to $1\n" . $herecurr);
 		}
 # check for module_init(), use category-specific init macros explicitly please
 		if ($line =~ /^module_init\s*\(/) {
-			WARN("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
+			ERROR("please use block_init(), type_init() etc. instead of module_init()\n" . $herecurr);
 		}
 # check for various ops structs, ensure they are const.
 		my $struct_ops = qr{AIOCBInfo|
@@ -2490,7 +2490,7 @@  sub process {
 				VMStateInfo}x;
 		if ($line !~ /\bconst\b/ &&
 		    $line =~ /\b($struct_ops)\b/) {
-			WARN("struct $1 should normally be const\n" .
+			ERROR("struct $1 should normally be const\n" .
 				$herecurr);
 		}
 
@@ -2500,14 +2500,14 @@  sub process {
 			$string = substr($rawline, $-[1], $+[1] - $-[1]);
 			$string =~ s/%%/__/g;
 			if ($string =~ /(?<!%)%L[udi]/) {
-				WARN("\%Ld/%Lu are not-standard C, use %lld/%llu\n" . $herecurr);
+				ERROR("\%Ld/%Lu are not-standard C, use %lld/%llu\n" . $herecurr);
 				last;
 			}
 		}
 
 # QEMU specific tests
 		if ($rawline =~ /\b(?:Qemu|QEmu)\b/) {
-			WARN("use QEMU instead of Qemu or QEmu\n" . $herecurr);
+			ERROR("use QEMU instead of Qemu or QEmu\n" . $herecurr);
 		}
 
 # Qemu error function tests
@@ -2521,7 +2521,7 @@  sub process {
 				error_report}x;
 
 	if ($rawline =~ /\b(?:$qemu_error_funcs)\s*\(\s*\".*\\n/) {
-		WARN("Error messages should not contain newlines\n" . $herecurr);
+		ERROR("Error messages should not contain newlines\n" . $herecurr);
 	}
 
 	# Continue checking for error messages that contains newlines. This
@@ -2542,7 +2542,7 @@  sub process {
 		}
 
 		if ($rawlines[$i] =~ /\b(?:$qemu_error_funcs)\s*\(/) {
-			WARN("Error messages should not contain newlines\n" . $herecurr);
+			ERROR("Error messages should not contain newlines\n" . $herecurr);
 		}
 	}