[PATCH] update checkpatch.pl to version 0.08

[PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

This version brings a number of new checks, and a number of bug
fixes.  Of note:

  - warnings for multiple assignments per line
  - warnings for multiple declarations per line
  - checks for single statement blocks with braces

This patch includes an update for feature-removal-schedule.txt to
better target checks.

Andy Whitcroft (12):
      Version: 0.08
      only apply printk checks where there is a string literal
      allow suppression of errors for when no patch is found
      warn about multiple assignments
      warn on declaration of multiple variables
      check for kfree() with needless null check
      check for single statement braced blocks
      check for aggregate initialisation on the next line
      handle the => operator
      check for spaces between function name and open parenthesis
      move to explicit Check: entries in feature-removal-schedule.txt
      handle pointer attributes

Signed-off-by: Andy Whitcroft <apw@shadowen.org>
---

	This has been tested, but not had quite its usual soak on
	the lkml flow.	It is being pushed out a little sooner than
	normal as I am going to be off the 'net for a week and wanted
	to get it out before.

diff --git a/Documentation/feature-removal-schedule.txt b/Documentation/feature-removal-schedule.txt
index 1e8dba0..85d736e 100644
--- a/Documentation/feature-removal-schedule.txt
+++ b/Documentation/feature-removal-schedule.txt
@@ -63,6 +63,7 @@ Who:	David Miller <davem@davemloft.net>
 What:	Video4Linux API 1 ioctls and video_decoder.h from Video devices.
 When:	December 2006
 Files:	include/linux/video_decoder.h
+Check:	include/linux/video_decoder.h
 Why:	V4L1 AP1 was replaced by V4L2 API. during migration from 2.4 to 2.6
 	series. The old API have lots of drawbacks and don't provide enough
 	means to work with all video and audio standards. The newer API is
@@ -79,7 +80,7 @@ Who:	Mauro Carvalho Chehab <mchehab@brturbo.com.br>
 What:	remove EXPORT_SYMBOL(kernel_thread)
 When:	August 2006
 Files:	arch/*/kernel/*_ksyms.c
-Funcs:	kernel_thread
+Check:	kernel_thread
 Why:	kernel_thread is a low-level implementation detail.  Drivers should
         use the <linux/kthread.h> API instead which shields them from
 	implementation details and provides a higherlevel interface that
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 25e20a2..73751ab 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -9,7 +9,7 @@ use strict;
 my $P = $0;
 $P =~ s@.*/@@g;
 
-my $V = '0.07';
+my $V = '0.08';
 
 use Getopt::Long qw(:config no_auto_abbrev);
 
@@ -47,16 +47,14 @@ my $removal = 'Documentation/feature-removal-schedule.txt';
 if ($tree && -f $removal) {
 	open(REMOVE, "<$removal") || die "$P: $removal: open failed - $!\n";
 	while (<REMOVE>) {
-		if (/^Files:\s+(.*\S)/) {
-			for my $file (split(/[, ]+/, $1)) {
-				if ($file =~ m@include/(.*)@) {
+		if (/^Check:\s+(.*\S)/) {
+			for my $entry (split(/[, ]+/, $1)) {
+				if ($entry =~ m@include/(.*)@) {
 					push(@dep_includes, $1);
-				}
-			}
 
-		} elsif (/^Funcs:\s+(.*\S)/) {
-			for my $func (split(/[, ]+/, $1)) {
-				push(@dep_functions, $func);
+				} elsif ($entry !~ m@/@) {
+					push(@dep_functions, $entry);
+				}
 			}
 		}
 	}
@@ -153,7 +151,7 @@ sub sanitise_line {
 }
 
 sub ctx_block_get {
-	my ($linenr, $remain, $outer, $open, $close) = @_;
+	my ($linenr, $remain, $outer, $open, $close, $off) = @_;
 	my $line;
 	my $start = $linenr - 1;
 	my $blk = '';
@@ -161,38 +159,58 @@ sub ctx_block_get {
 	my @c;
 	my @res = ();
 
+	my $level = 0;
 	for ($line = $start; $remain > 0; $line++) {
 		next if ($rawlines[$line] =~ /^-/);
 		$remain--;
 
 		$blk .= $rawlines[$line];
+		foreach my $c (split(//, $rawlines[$line])) {
+			##print "C<$c>L<$level><$open$close>O<$off>\n";
+			if ($off > 0) {
+				$off--;
+				next;
+			}
 
-		@o = ($blk =~ /$open/g);
-		@c = ($blk =~ /$close/g);
+			if ($c eq $close && $level > 0) {
+				$level--;
+				last if ($level == 0);
+			} elsif ($c eq $open) {
+				$level++;
+			}
+		}
 
-		if (!$outer || (scalar(@o) - scalar(@c)) == 1) {
+		if (!$outer || $level <= 1) {
 			push(@res, $rawlines[$line]);
 		}
 
-		last if (scalar(@o) == scalar(@c));
+		last if ($level == 0);
 	}
 
-	return @res;
+	return ($level, @res);
 }
 sub ctx_block_outer {
 	my ($linenr, $remain) = @_;
 
-	return ctx_block_get($linenr, $remain, 1, '\{', '\}');
+	my ($level, @r) = ctx_block_get($linenr, $remain, 1, '{', '}', 0);
+	return @r;
 }
 sub ctx_block {
 	my ($linenr, $remain) = @_;
 
-	return ctx_block_get($linenr, $remain, 0, '\{', '\}');
+	my ($level, @r) = ctx_block_get($linenr, $remain, 0, '{', '}', 0);
+	return @r;
 }
 sub ctx_statement {
+	my ($linenr, $remain, $off) = @_;
+
+	my ($level, @r) = ctx_block_get($linenr, $remain, 0, '(', ')', $off);
+	return @r;
+}
+sub ctx_block_level {
 	my ($linenr, $remain) = @_;
 
-	return ctx_block_get($linenr, $remain, 0, '\(', '\)');
+	return ctx_block_get($linenr, $remain, 0, '{', '}', 0);
 }
 
 sub ctx_locate_comment {
@@ -246,16 +264,23 @@ sub cat_vet {
 	return $vet;
 }
 
+my @report = ();
+sub report {
+	push(@report, $_[0]);
+}
+sub report_dump {
+	@report;
+}
 sub ERROR {
-	print "ERROR: $_[0]\n";
+	report("ERROR: $_[0]\n");
 	our $clean = 0;
 }
 sub WARN {
-	print "WARNING: $_[0]\n";
+	report("WARNING: $_[0]\n");
 	our $clean = 0;
 }
 sub CHK {
-	print "CHECK: $_[0]\n";
+	report("CHECK: $_[0]\n");
 	our $clean = 0;
 }
 
@@ -318,7 +343,10 @@ sub process {
 				(?:\s*\*+\s*const|\s*\*+)?
 			  }x;
 	my $Declare	= qr{(?:$Storage\s+)?$Type};
-	my $Attribute	= qr{__read_mostly|__init|__initdata};
+	my $Attribute	= qr{const|__read_mostly|__init|__initdata|__meminit};
+
+	my $Member	= qr{->$Ident|\.$Ident|\[[^]]*\]};
+	my $Lval	= qr{$Ident(?:$Member)*};
 
 	# Pre-scan the patch looking for any __setup documentation.
 	my @setup_docs = ();
@@ -509,7 +537,7 @@ sub process {
 # if/while/etc brace do not go on next line, unless defining a do while loop,
 # or if that brace on the next line is for something else
 		if ($line =~ /\b(?:(if|while|for|switch)\s*\(|do\b|else\b)/ && $line !~ /^.#/) {
-			my @ctx = ctx_statement($linenr, $realcnt);
+			my @ctx = ctx_statement($linenr, $realcnt, 0);
 			my $ctx_ln = $linenr + $#ctx + 1;
 			my $ctx_cnt = $realcnt - $#ctx - 1;
 			my $ctx = join("\n", @ctx);
@@ -521,7 +549,7 @@ sub process {
 			##warn "line<$line>\nctx<$ctx>\nnext<$lines[$ctx_ln - 1]>";
 
 			if ($ctx !~ /{\s*/ && $ctx_cnt > 0 && $lines[$ctx_ln - 1] =~ /^\+\s*{/) {
-				ERROR("That { should be on the previous line\n" .
+				ERROR("That open brace { should be on the previous line\n" .
 					"$here\n$ctx\n$lines[$ctx_ln - 1]");
 			}
 		}
@@ -535,6 +563,12 @@ sub process {
 			next;
 		}
 
+# check for initialisation to aggregates open brace on the next line
+		if ($prevline =~ /$Declare\s*$Ident\s*=\s*$/ &&
+		    $line =~ /^.\s*{/) {
+			ERROR("That open brace { should be on the previous line\n" . $hereprev);
+		}
+
 #
 # Checks which are anchored on the added line.
 #
@@ -570,8 +604,13 @@ sub process {
 			}
 		}
 
+# check for external initialisers.
+		if ($line =~ /^.$Type\s*$Ident\s*=\s*(0|NULL);/) {
+			ERROR("do not initialise externals to 0 or NULL\n" .
+				$herecurr);
+		}
 # check for static initialisers.
-		if ($line=~/\s*static\s.*=\s+(0|NULL);/) {
+		if ($line =~ /\s*static\s.*=\s*(0|NULL);/) {
 			ERROR("do not initialise statics to 0 or NULL\n" .
 				$herecurr);
 		}
@@ -593,11 +632,11 @@ sub process {
 			ERROR("\"(foo $1 )\" should be \"(foo $1)\"\n" .
 				$herecurr);
 
-		} elsif ($line =~ m{$NonptrType(\*+)(?:\s+const)?\s+[A-Za-z\d_]+}) {
+		} elsif ($line =~ m{$NonptrType(\*+)(?:\s+$Attribute)?\s+[A-Za-z\d_]+}) {
 			ERROR("\"foo$1 bar\" should be \"foo $1bar\"\n" .
 				$herecurr);
 
-		} elsif ($line =~ m{$NonptrType\s+(\*+)(?!\s+const)\s+[A-Za-z\d_]+}) {
+		} elsif ($line =~ m{$NonptrType\s+(\*+)(?!\s+$Attribute)\s+[A-Za-z\d_]+}) {
 			ERROR("\"foo $1 bar\" should be \"foo $1bar\"\n" .
 				$herecurr);
 		}
@@ -614,7 +653,7 @@ sub process {
 # to try and find and validate the current printk.  In summary the current
 # printk includes all preceeding printk's which have no newline on the end.
 # we assume the first bad printk is the one to report.
-		if ($line =~ /\bprintk\((?!KERN_)/) {
+		if ($line =~ /\bprintk\((?!KERN_)\s*"/) {
 			my $ok = 0;
 			for (my $ln = $linenr - 1; $ln >= $first_line; $ln--) {
 				#print "CHECK<$lines[$ln - 1]\n";
@@ -639,6 +678,12 @@ sub process {
 			ERROR("open brace '{' following function declarations go on the next line\n" . $herecurr);
 		}
 
+# check for spaces between functions and their parentheses.
+		if ($line =~ /($Ident)\s+\(/ &&
+		    $1 !~ /^(?:if|for|while|switch|return|volatile)$/ &&
+		    $line !~ /$Type\s+\(/ && $line !~ /^.\#\s*define\b/) {
+			ERROR("no space between function name and open parenthesis '('\n" . $herecurr);
+		}
 # Check operator spacing.
 		# Note we expand the line with the leading + as the real
 		# line will be displayed with the leading + and the tabs
@@ -647,7 +692,7 @@ sub process {
 		$opline = expand_tabs($opline);
 		$opline =~ s/^./ /;
 		if (!($line=~/\#\s*include/)) {
-			my @elements = split(/(<<=|>>=|<=|>=|==|!=|\+=|-=|\*=|\/=|%=|\^=|\|=|&=|->|<<|>>|<|>|=|!|~|&&|\|\||,|\^|\+\+|--|;|&|\||\+|-|\*|\/\/|\/)/, $opline);
+			my @elements = split(/(<<=|>>=|<=|>=|==|!=|\+=|-=|\*=|\/=|%=|\^=|\|=|&=|=>|->|<<|>>|<|>|=|!|~|&&|\|\||,|\^|\+\+|--|;|&|\||\+|-|\*|\/\/|\/)/, $opline);
 			my $off = 0;
 			for (my $n = 0; $n < $#elements; $n += 2) {
 				$off += length($elements[$n]);
@@ -773,6 +818,18 @@ sub process {
 			}
 		}
 
+# check for multiple assignments
+		if ($line =~ /^.\s*$Lval\s*=\s*$Lval\s*=(?!=)/) {
+			WARN("multiple assignments should be avoided\n" . $herecurr);
+		}
+
+# check for multiple declarations, allowing for a function declaration
+# continuation.
+		if ($line =~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Ident.*/ &&
+		    $line !~ /^.\s*$Type\s+$Ident(?:\s*=[^,{]*)?\s*,\s*$Type\s*$Ident.*/) {
+			WARN("declaring multiple variables together should be avoided\n" . $herecurr);
+		}
+
 #need space before brace following if, while, etc
 		if ($line =~ /\(.*\){/ || $line =~ /do{/) {
 			ERROR("need a space before the open brace '{'\n" . $herecurr);
@@ -847,13 +904,18 @@ sub process {
 			# or the one below.
 			my $ln = $linenr;
 			my $cnt = $realcnt;
+			my $off = 0;
 
-			# If the macro starts on the define line start there.
-			if ($prevline !~ m{^.#\s*define\s*$Ident(?:\([^\)]*\))?\s*\\\s*$}) {
+			# If the macro starts on the define line start
+			# grabbing the statement after the identifier
+			$prevline =~ m{^(.#\s*define\s*$Ident(?:\([^\)]*\))?\s*)(.*)\\\s*$};
+			##print "1<$1> 2<$2>\n";
+			if ($2 ne '') {
+				$off = length($1);
 				$ln--;
 				$cnt++;
 			}
-			my @ctx = ctx_statement($ln, $cnt);
+			my @ctx = ctx_statement($ln, $cnt, $off);
 			my $ctx_ln = $ln + $#ctx + 1;
 			my $ctx = join("\n", @ctx);
 
@@ -873,6 +935,44 @@ sub process {
 			}
 		}
 
+# check for redundant bracing round if etc
+		if ($line =~ /\b(if|while|for|else)\b/) {
+			# Locate the end of the opening statement.
+			my @control = ctx_statement($linenr, $realcnt, 0);
+			my $nr = $linenr + (scalar(@control) - 1);
+			my $cnt = $realcnt - (scalar(@control) - 1);
+
+			my $off = $realcnt - $cnt;
+			#print "$off: line<$line>end<" . $lines[$nr - 1] . ">\n";
+
+			# If this is is a braced statement group check it
+			if ($lines[$nr - 1] =~ /{\s*$/) {
+				my ($lvl, @block) = ctx_block_level($nr, $cnt);
+
+				my $stmt = join(' ', @block);
+				$stmt =~ s/^[^{]*{//;
+				$stmt =~ s/}[^}]*$//;
+
+				#print "block<" . join(' ', @block) . "><" . scalar(@block) . ">\n";
+				#print "stmt<$stmt>\n\n";
+
+				# Count the ;'s if there is fewer than two
+				# then there can only be one statement,
+				# if there is a brace inside we cannot
+				# trivially detect if its one statement.
+				# Also nested if's often require braces to
+				# disambiguate the else binding so shhh there.
+				my @semi = ($stmt =~ /;/g);
+				##print "semi<" . scalar(@semi) . ">\n";
+				if ($lvl == 0 && scalar(@semi) < 2 &&
+				    $stmt !~ /{/ && $stmt !~ /\bif\b/) {
+				    	my $herectx = "$here\n" . join("\n", @control, @block[1 .. $#block]) . "\n";
+				    	shift(@block);
+					ERROR("braces {} are not necessary for single statement blocks\n" . $herectx);
+				}
+			}
+		}
+
 # don't include deprecated include files (uses RAW line)
 		for my $inc (@dep_includes) {
 			if ($rawline =~ m@\#\s*include\s*\<$inc>@) {
@@ -898,6 +998,14 @@ sub process {
 				$herecurr);
 		}
 
+# check for needless kfree() checks
+		if ($prevline =~ /\bif\s*\(([^\)]*)\)/) {
+			my $expr = $1;
+			if ($line =~ /\bkfree\(\Q$expr\E\);/) {
+				WARN("kfree(NULL) is safe this check is probabally not required\n" . $hereprev);
+			}
+		}
+
 # warn about #ifdefs in C files
 #		if ($line =~ /^.#\s*if(|n)def/ && ($realfile =~ /\.c$/)) {
 #			print "#ifdef in C files should be avoided\n";
@@ -952,6 +1060,9 @@ sub process {
 		ERROR("Missing Signed-off-by: line(s)\n");
 	}
 
+	if ($clean == 0 && ($chk_patch || $is_patch)) {
+		print report_dump();
+	}
 	if ($clean == 1 && $quiet == 0) {
 		print "Your patch has no obvious style problems and is ready for submission.\n"
 	}

Re: [PATCH] update checkpatch.pl to version 0.08

[Kok, Auke]

Andy Whitcroft wrote:
> This version brings a number of new checks, and a number of bug
> fixes.  Of note:
> 
>   - warnings for multiple assignments per line


This is bugged. e.g. the following line will hit this exception check:

	int i = some_function(a, b, c);



>   - warnings for multiple declarations per line
>   - checks for single statement blocks with braces
> 
> This patch includes an update for feature-removal-schedule.txt to
> better target checks.
> 
> Andy Whitcroft (12):
>       check for single statement braced blocks
> 
> Signed-off-by: Andy Whitcroft <apw@shadowen.org>
> ---
> 
> +# check for redundant bracing round if etc
> +		if ($line =~ /\b(if|while|for|else)\b/) {
> +			# Locate the end of the opening statement.
> +			my @control = ctx_statement($linenr, $realcnt, 0);
> +			my $nr = $linenr + (scalar(@control) - 1);
> +			my $cnt = $realcnt - (scalar(@control) - 1);
> +
> +			my $off = $realcnt - $cnt;
> +			#print "$off: line<$line>end<" . $lines[$nr - 1] . ">\n";
> +
> +			# If this is is a braced statement group check it
> +			if ($lines[$nr - 1] =~ /{\s*$/) {
> +				my ($lvl, @block) = ctx_block_level($nr, $cnt);
> +
> +				my $stmt = join(' ', @block);
> +				$stmt =~ s/^[^{]*{//;
> +				$stmt =~ s/}[^}]*$//;
> +
> +				#print "block<" . join(' ', @block) . "><" . scalar(@block) . ">\n";
> +				#print "stmt<$stmt>\n\n";
> +
> +				# Count the ;'s if there is fewer than two
> +				# then there can only be one statement,
> +				# if there is a brace inside we cannot
> +				# trivially detect if its one statement.
> +				# Also nested if's often require braces to
> +				# disambiguate the else binding so shhh there.
> +				my @semi = ($stmt =~ /;/g);
> +				##print "semi<" . scalar(@semi) . ">\n";
> +				if ($lvl == 0 && scalar(@semi) < 2 &&
> +				    $stmt !~ /{/ && $stmt !~ /\bif\b/) {
> +				    	my $herectx = "$here\n" . join("\n", @control, @block[1 .. $#block]) . "\n";
> +				    	shift(@block);
> +					ERROR("braces {} are not necessary for single statement blocks\n" . $herectx);


This is a royal pain, since it now throws an ERROR for the obviously preferable 
piece of code below:

if (err) {
	do_something();
	return -ERR;
} else {
	do_somthing_else();
}



Also, CondingStyle explicitly permits this style (even encourages it):

---
Do not unnecessarily use braces where a single statement will do.

if (condition)
	action();

This does not apply if one branch of a conditional statement is a single
statement. Use braces in both branches.

if (condition) {
	do_this();
	do_that();
} else {
	otherwise();
}
---

So, IMO this test needs to go, unless the script becomes smart enough to know 
that either side of the else requires braces. It's definately not an ERROR.

Cheers,

Auke

Re: [PATCH] update checkpatch.pl to version 0.08

[Jesper Juhl]

On 15/07/07, Andy Whitcroft <apw@shadowen.org> wrote:
>
> This version brings a number of new checks, and a number of bug
> fixes.  Of note:
>
>   - warnings for multiple assignments per line
>   - warnings for multiple declarations per line
>   - checks for single statement blocks with braces
>
> This patch includes an update for feature-removal-schedule.txt to
> better target checks.
>
> Andy Whitcroft (12):
>       Version: 0.08
>       only apply printk checks where there is a string literal
>       allow suppression of errors for when no patch is found
>       warn about multiple assignments
>       warn on declaration of multiple variables
>       check for kfree() with needless null check

It's not just kfree(), it's a whole list of functions, like kfree(),
vfree(), crypto_free_tfm() and a bunch of other freeing functions...


>       check for single statement braced blocks

Hmmm, if the single statement is a macro, braces sometimes make a lot
of sense...


Suggested further improvement; check that patches don't modify files
listed in Documentation/dontdiff


-- 
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html

Re: [PATCH] update checkpatch.pl to version 0.08

[Kok, Auke]

Andy Whitcroft wrote:
> This version brings a number of new checks, and a number of bug
> fixes.  Of note:
> 
>   - warnings for multiple assignments per line
>   - warnings for multiple declarations per line
>   - checks for single statement blocks with braces
> 
> This patch includes an update for feature-removal-schedule.txt to
> better target checks.
> 
> Andy Whitcroft (12):
>       Version: 0.08
>       check for spaces between function name and open parenthesis
> 

> +# check for spaces between functions and their parentheses.
> +		if ($line =~ /($Ident)\s+\(/ &&
> +		    $1 !~ /^(?:if|for|while|switch|return|volatile)$/ &&
> +		    $line !~ /$Type\s+\(/ && $line !~ /^.\#\s*define\b/) {
> +			ERROR("no space between function name and open parenthesis '('\n" . $herecurr);
> +		}


this somehow seems to match something completely non-related (a function pointer 
declaration case):

ERROR: no space between function name and open parenthesis '('
#7278: FILE: drivers/net/e1000e/hw.h:434:
+       bool (*check_mng_mode)(struct e1000_hw *);

even if I put a space between ")(", it still complains.


Auke

Re: [PATCH] update checkpatch.pl to version 0.08

[Kok, Auke]

Andy Whitcroft wrote:
> This version brings a number of new checks, and a number of bug
> fixes.  Of note:
> 
>   - warnings for multiple assignments per line
>   - warnings for multiple declarations per line
>   - checks for single statement blocks with braces
> 
> This patch includes an update for feature-removal-schedule.txt to
> better target checks.
> 
> Andy Whitcroft (12):
>       Version: 0.08
>       only apply printk checks where there is a string literal
>       allow suppression of errors for when no patch is found
>       warn about multiple assignments
>       warn on declaration of multiple variables
>       check for kfree() with needless null check
>       check for single statement braced blocks
>       check for aggregate initialisation on the next line
>       handle the => operator
>       check for spaces between function name and open parenthesis
>       move to explicit Check: entries in feature-removal-schedule.txt
>       handle pointer attributes

within the last 3 weeks, this script went from *really usable* to *a big noise 
maker*.

I am testing this with new drivers (igb, e1000e, ixgbe) and the amount of 
warnings it throws was in the order of 10 last week, but now I'm at hundreds 
again, and my code has not changed.

The superfluous braces error should definately be fixed.

Warning on multiple declarations on a line is nice, but IMO really too verbose 
(why is "int i, j;" bad? Did C somehow change syntax today?).

Some of the new features are plain broken as I posted before. A lot of it now is 
purely false positives only.

Bottom line: I really wish that I could have the script run in the old behaviour 
before. While this level of verbosity is great for single-line patches, it 
really completely wastes my time when I'm trying to get a grasp for a 200k hunk 
piece of code.

The best way to implement this is that I can somehow select / omit some of these 
checks when running the script. With more and more checks added to the script it 
will be very quick for new driver writers to stop using it because of the sheer 
volume that the script currently outputs.

Auke

Re: [PATCH] update checkpatch.pl to version 0.08

[Randy Dunlap]

On Mon, 23 Jul 2007 16:08:26 -0700 Kok, Auke wrote:

> Andy Whitcroft wrote:
> > This version brings a number of new checks, and a number of bug
> > fixes.  Of note:
> > 
> >   - warnings for multiple assignments per line
> 
> 
> This is bugged. e.g. the following line will hit this exception check:
> 
> 	int i = some_function(a, b, c);

Agreed.

...

> This is a royal pain, since it now throws an ERROR for the obviously preferable 
> piece of code below:
> 
> if (err) {
> 	do_something();
> 	return -ERR;
> } else {
> 	do_somthing_else();
> }
> 
> 
> 
> Also, CondingStyle explicitly permits this style (even encourages it):

Yes, Linus has recently written that he prefers that style also.

The problem (to me at least) is that this patch:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e659ba4a0d2d471c0d73590f78e1a1b5a1eede48

was not discussed before it was merged.  It was discussed a little bit
after it was merged.  Basically we have no concensus on this patch,
just a dictate (which we can all live with).



> ---
> Do not unnecessarily use braces where a single statement will do.
> 
> if (condition)
> 	action();
> 
> This does not apply if one branch of a conditional statement is a single
> statement. Use braces in both branches.
> 
> if (condition) {
> 	do_this();
> 	do_that();
> } else {
> 	otherwise();
> }
> ---
> 
> So, IMO this test needs to go, unless the script becomes smart enough to know 
> that either side of the else requires braces. It's definately not an ERROR.


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Kok, Auke wrote:
> Andy Whitcroft wrote:
>> This version brings a number of new checks, and a number of bug
>> fixes.  Of note:
>>
>>   - warnings for multiple assignments per line
> 
> 
> This is bugged. e.g. the following line will hit this exception check:
> 
>     int i = some_function(a, b, c);

Yep that is plainly wrong.  Will sort that one out.

>>   - warnings for multiple declarations per line
>>   - checks for single statement blocks with braces
>>
>> This patch includes an update for feature-removal-schedule.txt to
>> better target checks.
>>
>> Andy Whitcroft (12):
>>       check for single statement braced blocks
>>
>> Signed-off-by: Andy Whitcroft <apw@shadowen.org>
>> ---
>>
>> +# check for redundant bracing round if etc
>> +        if ($line =~ /\b(if|while|for|else)\b/) {
>> +            # Locate the end of the opening statement.
>> +            my @control = ctx_statement($linenr, $realcnt, 0);
>> +            my $nr = $linenr + (scalar(@control) - 1);
>> +            my $cnt = $realcnt - (scalar(@control) - 1);
>> +
>> +            my $off = $realcnt - $cnt;
>> +            #print "$off: line<$line>end<" . $lines[$nr - 1] . ">\n";
>> +
>> +            # If this is is a braced statement group check it
>> +            if ($lines[$nr - 1] =~ /{\s*$/) {
>> +                my ($lvl, @block) = ctx_block_level($nr, $cnt);
>> +
>> +                my $stmt = join(' ', @block);
>> +                $stmt =~ s/^[^{]*{//;
>> +                $stmt =~ s/}[^}]*$//;
>> +
>> +                #print "block<" . join(' ', @block) . "><" .
>> scalar(@block) . ">\n";
>> +                #print "stmt<$stmt>\n\n";
>> +
>> +                # Count the ;'s if there is fewer than two
>> +                # then there can only be one statement,
>> +                # if there is a brace inside we cannot
>> +                # trivially detect if its one statement.
>> +                # Also nested if's often require braces to
>> +                # disambiguate the else binding so shhh there.
>> +                my @semi = ($stmt =~ /;/g);
>> +                ##print "semi<" . scalar(@semi) . ">\n";
>> +                if ($lvl == 0 && scalar(@semi) < 2 &&
>> +                    $stmt !~ /{/ && $stmt !~ /\bif\b/) {
>> +                        my $herectx = "$here\n" . join("\n",
>> @control, @block[1 .. $#block]) . "\n";
>> +                        shift(@block);
>> +                    ERROR("braces {} are not necessary for single
>> statement blocks\n" . $herectx);
> 
> 
> This is a royal pain, since it now throws an ERROR for the obviously
> preferable piece of code below:
> 
> if (err) {
>     do_something();
>     return -ERR;
> } else {
>     do_somthing_else();
> }

Hmmm, is that obviouly nicer than the below?  Its fully a line longer
for no benefit.  But ignoring that, this seems to have snuck in to
CodingStyle hmmm ... will see what I can do if anything to stop these
being picked up I guess.

	if (err) {
		do_something();
		return -ERR;
	} else
		do_something_else();

Andrew, as you merged the change to CodingStyle I'll take that as your
being ok with these being accepted.

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[Andrew Morton]

On Tue, 24 Jul 2007 10:06:51 +0100 Andy Whitcroft <apw@shadowen.org> wrote:

> > This is a royal pain, since it now throws an ERROR for the obviously
> > preferable piece of code below:
> > 
> > if (err) {
> >     do_something();
> >     return -ERR;
> > } else {
> >     do_somthing_else();
> > }
> 
> Hmmm, is that obviouly nicer than the below?  Its fully a line longer
> for no benefit.  But ignoring that, this seems to have snuck in to
> CodingStyle hmmm ... will see what I can do if anything to stop these
> being picked up I guess.
> 
> 	if (err) {
> 		do_something();
> 		return -ERR;
> 	} else
> 		do_something_else();

The kool kids on linux-usb-devel largely ended up deciding that the second
version looks dorky.

Especially if there's a comment over do_something_else(), and if there's
not a comment, perhaps there should be?

> Andrew, as you merged the change to CodingStyle I'll take that as your
> being ok with these being accepted.

It's very marginal and is sure to get people hot and bothered.  I'd suggest
that checkpatch be neutral on that.

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Andrew Morton wrote:
> On Tue, 24 Jul 2007 10:06:51 +0100 Andy Whitcroft <apw@shadowen.org> wrote:
> 
>>> This is a royal pain, since it now throws an ERROR for the obviously
>>> preferable piece of code below:
>>>
>>> if (err) {
>>>     do_something();
>>>     return -ERR;
>>> } else {
>>>     do_somthing_else();
>>> }
>> Hmmm, is that obviouly nicer than the below?  Its fully a line longer
>> for no benefit.  But ignoring that, this seems to have snuck in to
>> CodingStyle hmmm ... will see what I can do if anything to stop these
>> being picked up I guess.
>>
>> 	if (err) {
>> 		do_something();
>> 		return -ERR;
>> 	} else
>> 		do_something_else();
> 
> The kool kids on linux-usb-devel largely ended up deciding that the second
> version looks dorky.
> 
> Especially if there's a comment over do_something_else(), and if there's
> not a comment, perhaps there should be?
> 
>> Andrew, as you merged the change to CodingStyle I'll take that as your
>> being ok with these being accepted.
> 
> It's very marginal and is sure to get people hot and bothered.  I'd suggest
> that checkpatch be neutral on that.

Ok, now if either the preceeding block or following block has {}'s then
we don't report this block for being one line long.  We will miss some
this way, but hey.

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Kok, Auke wrote:
> Andy Whitcroft wrote:
>> This version brings a number of new checks, and a number of bug
>> fixes.  Of note:
>>
>>   - warnings for multiple assignments per line
>>   - warnings for multiple declarations per line
>>   - checks for single statement blocks with braces
>>
>> This patch includes an update for feature-removal-schedule.txt to
>> better target checks.
>>
>> Andy Whitcroft (12):
>>       Version: 0.08
>>       only apply printk checks where there is a string literal
>>       allow suppression of errors for when no patch is found
>>       warn about multiple assignments
>>       warn on declaration of multiple variables
>>       check for kfree() with needless null check
>>       check for single statement braced blocks
>>       check for aggregate initialisation on the next line
>>       handle the => operator
>>       check for spaces between function name and open parenthesis
>>       move to explicit Check: entries in feature-removal-schedule.txt
>>       handle pointer attributes
> 
> within the last 3 weeks, this script went from *really usable* to *a big
> noise maker*.

She is developing for sure.  I for one don't want it to be worthless.  I
also want it to catch the things that Andrew is hottest on.  A difficult
path.  Always remember that this is a guide to style not definitive.

> I am testing this with new drivers (igb, e1000e, ixgbe) and the amount
> of warnings it throws was in the order of 10 last week, but now I'm at
> hundreds again, and my code has not changed.
> 
> The superfluous braces error should definately be fixed.

Yes, that was a misunderstanding my end, and I have loosened that check.
for the next version.  Not sure if its much use anymore but it should no
longer winge all over your patch.

> Warning on multiple declarations on a line is nice, but IMO really too
> verbose (why is "int i, j;" bad? Did C somehow change syntax today?).

No the normal response is two fold:

1) "what the heck are i and j those are meaningless names"
2) "please can we have some comments for those variables"

which normally leads to the suggestion that it be the following form:

	int source;		/* source clock hand */
	int destination;	/* destination clock hand */

and all is well.  That was the background for the checks.  However,
there is much upsetedness over it and push for i, j, k, l being a handy
form.

I am inclined to drop this check completely.  Andrew this was one of
your requests?

> Some of the new features are plain broken as I posted before. A lot of
> it now is purely false positives only.
> 
> Bottom line: I really wish that I could have the script run in the old
> behaviour before. While this level of verbosity is great for single-line
> patches, it really completely wastes my time when I'm trying to get a
> grasp for a 200k hunk piece of code.

I can only shudder at the thought of a 200k patch, but ok.

> The best way to implement this is that I can somehow select / omit some
> of these checks when running the script. With more and more checks added
> to the script it will be very quick for new driver writers to stop using
> it because of the sheer volume that the script currently outputs.

Yeah I have been feeling that we might want to say "--no-check" etc so
you can only get the more serious errors etc.  Will think on that.

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[Ingo Molnar]

* Andy Whitcroft <apw@shadowen.org> wrote:

> > within the last 3 weeks, this script went from *really usable* to *a 
> > big noise maker*.

seconded ...

v0.06 was "almost there". I fixed kernel/sched.c to be completely clean, 
except for 3 false positives. That was a real improvement, and i started 
to like checkpatch.pl.

v0.08 is a clear step backwards: it emits 61 warnings now, 90% of which 
are totally bogus. The only 'fix' for many of those warnings is to make 
the code _worse_. That is unacceptable.

> > Warning on multiple declarations on a line is nice, but IMO really 
> > too verbose (why is "int i, j;" bad? Did C somehow change syntax 
> > today?).
> 
> No the normal response is two fold:
> 
> 1) "what the heck are i and j those are meaningless names"
> 2) "please can we have some comments for those variables"

you really should not even be arguing about this. LOOK AT the many false 
positives in sched.c. This is perfectly readable code:

 void __init sched_init(void)
 {
         u64 now = sched_clock();
         int highest_cpu = 0;
         int i, j;

         for_each_possible_cpu(i) {
                 struct rt_prio_array *array;
                 struct rq *rq;

                 rq = cpu_rq(i);

this warning for "i, j" is clearly bogus. So are many of the other 
warnings. checkpatch.pl went from a useful tool that improved the 
quality of the kernel to a rigid, unflexible policeman. It needs to be 
fixed or needs to be gotten rid of.

> which normally leads to the suggestion that it be the following form:
> 
> 	int source;		/* source clock hand */
> 	int destination;	/* destination clock hand */

what the hell are you thinking? Not every trivial line of code needs to 
be commented. Comments are needed for the _nontrivial_ lines of code, 
and there's no way a tool can decide that. The longer "destination" 
variable suggested by you can _easily_ make a previously readable piece 
of code unreadable.

	Ingo

Re: [PATCH] update checkpatch.pl to version 0.08

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> what the hell are you thinking? Not every trivial line of code needs 
> to be commented. Comments are needed for the _nontrivial_ lines of 
> code, and there's no way a tool can decide that. [...]

and i thought you understood this point, as earlier versions of 
checkpatch.pl were carefully filtering out _known nontrivial_ code, such 
as the barrier APIs (smp_rmb(), etc.) where we know that they are 
nontrivial in _100%_ of the cases.

trying to warn about code that 'might' be unclean is a catastrophy. This 
tool should only make noise if it is _really_ sure that there's 
something wrong going on. If the tool cannot make an intelligent 
decision about it then it should be silent!

Btw., there's an easy solution for v0.08's many breakages: turn off the 
WARNING output by default and rename it to 'INFO' or something other 
non-alarming.

	Ingo

Re: [PATCH] update checkpatch.pl to version 0.08

[Dmitry Torokhov]

On 7/24/07, Andy Whitcroft <apw@shadowen.org> wrote:
> Andrew Morton wrote:
> > On Tue, 24 Jul 2007 10:06:51 +0100 Andy Whitcroft <apw@shadowen.org> wrote:
> >
> >>> This is a royal pain, since it now throws an ERROR for the obviously
> >>> preferable piece of code below:
> >>>
> >>> if (err) {
> >>>     do_something();
> >>>     return -ERR;
> >>> } else {
> >>>     do_somthing_else();
> >>> }
> >> Hmmm, is that obviouly nicer than the below?  Its fully a line longer
> >> for no benefit.  But ignoring that, this seems to have snuck in to
> >> CodingStyle hmmm ... will see what I can do if anything to stop these
> >> being picked up I guess.
> >>
> >>      if (err) {
> >>              do_something();
> >>              return -ERR;
> >>      } else
> >>              do_something_else();
> >
> > The kool kids on linux-usb-devel largely ended up deciding that the second
> > version looks dorky.
> >
> > Especially if there's a comment over do_something_else(), and if there's
> > not a comment, perhaps there should be?
> >
> >> Andrew, as you merged the change to CodingStyle I'll take that as your
> >> being ok with these being accepted.
> >
> > It's very marginal and is sure to get people hot and bothered.  I'd suggest
> > that checkpatch be neutral on that.
>
> Ok, now if either the preceeding block or following block has {}'s then
> we don't report this block for being one line long.  We will miss some
> this way, but hey.
>

It also complains on the following:

+       if (retval && !--handle->open) {
+               /*
+                * Make sure we are not delivering any more events
+                * through this handle
+                */
+               synchronize_sched();
+       }

There is no way I'll drop braces there. You should probably not
exclude comments from line count when making decision if braces are
needed.

-- 
Dmitry

Re: [PATCH] update checkpatch.pl to version 0.08

[jschopp]

> within the last 3 weeks, this script went from *really usable* to *a big 
> noise maker*.

As we (mostly Andy of late) add more checks (good) there is bound to be some code we just 
didn't forsee that generates false positives (bad).  You can see a consistent history of 
cleaning these up as quickly as people send them in.  Hopefully in the interim there 
aren't too many false positives and the script is still useful.  We do try to put the new 
tests through their paces before adding them in, but our imaginations are limited.

The goal has always been to err on the side of missing badness in code to avoid false 
positives. This way, when there is output it has a very high chance of not wasting your 
time.  Wait a couple weeks and it'll be there again.

> Bottom line: I really wish that I could have the script run in the old 
> behaviour before. While this level of verbosity is great for single-line 
> patches, it really completely wastes my time when I'm trying to get a 
> grasp for a 200k hunk piece of code.

I think it would be a great idea to have the script default to very conservative behavior 
and have a flag say --verbose to turn on checks that have a higher false positive rate 
(such as the multiple variable declarations per line).  This might also be a staging area 
for newer checks to get a chance to work out some kinks before being added to the default.

Re: [PATCH] update checkpatch.pl to version 0.08

[Adrian Bunk]

On Tue, Jul 24, 2007 at 08:58:25AM -0500, jschopp wrote:
>> within the last 3 weeks, this script went from *really usable* to *a big 
>> noise maker*.
>
> As we (mostly Andy of late) add more checks (good) there is bound to be 
> some code we just didn't forsee that generates false positives (bad).  You 
> can see a consistent history of cleaning these up as quickly as people send 
> them in.  Hopefully in the interim there aren't too many false positives 
> and the script is still useful.  We do try to put the new tests through 
> their paces before adding them in, but our imaginations are limited.
>
> The goal has always been to err on the side of missing badness in code to 
> avoid false positives. This way, when there is output it has a very high 
> chance of not wasting your time.  Wait a couple weeks and it'll be there 
> again.
>...

And it will be known as "noise maker" for years, even if that'll be 
fixed in a few weeks...

Running it on the latest -rc or -mm should usually give good hints 
whether the output has become better or worse.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Adrian Bunk wrote:
> On Tue, Jul 24, 2007 at 08:58:25AM -0500, jschopp wrote:
>>> within the last 3 weeks, this script went from *really usable* to *a big 
>>> noise maker*.
>> As we (mostly Andy of late) add more checks (good) there is bound to be 
>> some code we just didn't forsee that generates false positives (bad).  You 
>> can see a consistent history of cleaning these up as quickly as people send 
>> them in.  Hopefully in the interim there aren't too many false positives 
>> and the script is still useful.  We do try to put the new tests through 
>> their paces before adding them in, but our imaginations are limited.
>>
>> The goal has always been to err on the side of missing badness in code to 
>> avoid false positives. This way, when there is output it has a very high 
>> chance of not wasting your time.  Wait a couple weeks and it'll be there 
>> again.
>> ...
> 
> And it will be known as "noise maker" for years, even if that'll be 
> fixed in a few weeks...
> 
> Running it on the latest -rc or -mm should usually give good hints 
> whether the output has become better or worse.

I generally run a new release against all incoming patches on lkml for a
few days before releasing.  The latest problem ones have been caused by
a difference of opinion on what the CodingStyle means or about what the
"best" style for a few things.  Multiple initialisation etc being good
examples.

There is no way to test for "what the majority will dissagree with".

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[Jan Engelhardt]

On Jul 24 2007 12:19, Andy Whitcroft wrote:
>>>> if (err) {
>>>>     do_something();
>>>>     return -ERR;
>>>> } else {
>>>>     do_somthing_else();
>>>> }
>>>
>>> 	if (err) {
>>> 		do_something();
>>> 		return -ERR;
>>> 	} else
>>> 		do_something_else();
>> 
>> The kool kids on linux-usb-devel largely ended up deciding that the second
>> version looks dorky.
>
>Ok, now if either the preceeding block or following block has {}'s then
>we don't report this block for being one line long.  We will miss some
>this way, but hey.

As per Ingo Molnar [ http://lkml.org/lkml/2006/9/5/68 ],
all blocks in an if-else 'tree' should be {} if there is at least one
with more than two statements. (And I do not disagree.)


	Jan
-- 

Re: [PATCH] update checkpatch.pl to version 0.08

[Jan Engelhardt]

On Jul 23 2007 16:36, Kok, Auke wrote:
>
> this somehow seems to match something completely non-related (a function
> pointer declaration case):
>
> ERROR: no space between function name and open parenthesis '('
> #7278: FILE: drivers/net/e1000e/hw.h:434:
> +       bool (*check_mng_mode)(struct e1000_hw *);
>
> even if I put a space between ")(", it still complains.

[assumed] majority of kernel code uses ")(" instead of ")\s+(",
so the warning seems bogus. And it even gets nitpicky.



	Jan
-- 

Re: [PATCH] update checkpatch.pl to version 0.08

[Jan Engelhardt]

On Jul 24 2007 12:33, Andy Whitcroft wrote:
>
>> Warning on multiple declarations on a line is nice, but IMO really too
>> verbose (why is "int i, j;" bad? Did C somehow change syntax today?).
>
>No the normal response is two fold:
>
>1) "what the heck are i and j those are meaningless names"

Can we at least assume the submitter is sane in some ways?
i and j are picked for obvious iterater values - you would not want
verbosify that to fruit_iterator and process_iterator or whatever
because it's a hell lot more typing.
It takes more than a few Perl regexes to actually grasp the semantics
of whether "i" is useful or not.


	Jan
-- 

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Jan Engelhardt wrote:
> On Jul 23 2007 16:36, Kok, Auke wrote:
>> this somehow seems to match something completely non-related (a function
>> pointer declaration case):
>>
>> ERROR: no space between function name and open parenthesis '('
>> #7278: FILE: drivers/net/e1000e/hw.h:434:
>> +       bool (*check_mng_mode)(struct e1000_hw *);
>>
>> even if I put a space between ")(", it still complains.
> 
> [assumed] majority of kernel code uses ")(" instead of ")\s+(",
> so the warning seems bogus. And it even gets nitpicky.

Yep, thats just a false positive.  Will sort it out.

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[Randy Dunlap]

On Tue, 24 Jul 2007 18:51:39 +0200 (CEST) Jan Engelhardt wrote:

> 
> On Jul 24 2007 12:19, Andy Whitcroft wrote:
> >>>> if (err) {
> >>>>     do_something();
> >>>>     return -ERR;
> >>>> } else {
> >>>>     do_somthing_else();
> >>>> }
> >>>
> >>> 	if (err) {
> >>> 		do_something();
> >>> 		return -ERR;
> >>> 	} else
> >>> 		do_something_else();
> >> 
> >> The kool kids on linux-usb-devel largely ended up deciding that the second
> >> version looks dorky.
> >
> >Ok, now if either the preceeding block or following block has {}'s then
> >we don't report this block for being one line long.  We will miss some
> >this way, but hey.
> 
> As per Ingo Molnar [ http://lkml.org/lkml/2006/9/5/68 ],
> all blocks in an if-else 'tree' should be {} if there is at least one
> with more than two statements. (And I do not disagree.)

You are actually referring to this commit:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e659ba4a0d2d471c0d73590f78e1a1b5a1eede48

which did not appear on lkml for review... :(

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] update checkpatch.pl to version 0.08

[Paul Mundt]

On Tue, Jul 24, 2007 at 02:15:26AM -0700, Andrew Morton wrote:
> On Tue, 24 Jul 2007 10:06:51 +0100 Andy Whitcroft <apw@shadowen.org> wrote:
> 
> > > This is a royal pain, since it now throws an ERROR for the obviously
> > > preferable piece of code below:
> > > 
> > > if (err) {
> > >     do_something();
> > >     return -ERR;
> > > } else {
> > >     do_somthing_else();
> > > }
> > 
> > Hmmm, is that obviouly nicer than the below?  Its fully a line longer
> > for no benefit.  But ignoring that, this seems to have snuck in to
> > CodingStyle hmmm ... will see what I can do if anything to stop these
> > being picked up I guess.
> > 
> > 	if (err) {
> > 		do_something();
> > 		return -ERR;
> > 	} else
> > 		do_something_else();
> 
> The kool kids on linux-usb-devel largely ended up deciding that the second
> version looks dorky.
> 
Since when did linux-usb-devel decide stylistic nits of the rest of the
kernel? Let them do what they want in drivers/usb, I have a hard time
accepting that what someone arbitrarily decides they want to do in their
directory suddenly blanketly applies to the rest of the kernel, and
therefore warrants a CodingStyle update.

Perhaps CodingStyle can start being versioned, so people can opt out of
certain 'improvements' whenever someone has a vision, much like some
nameless licenses.

Personally I prefer the second style, and if there's a comment block,
then it makes sense to complete the tree with {}'s (the keyword here is
prefer, as it's a personal preference). checkpatch has been quite useful
for catching obviously broken things, and now it seems like it's just
overreaching. Perhaps this functionality can be split in to a lite
checkpatch for catching show-stoppers for application and then something
more akin to a CodingStyle validator for the folks interested in
arbitrarily defining convention, which they can use freely while the rest
of us try to get something useful done.

Re: [PATCH] update checkpatch.pl to version 0.08

[Jan Engelhardt]

On Jul 24 2007 10:20, Randy Dunlap wrote:
>> As per Ingo Molnar [ http://lkml.org/lkml/2006/9/5/68 ],
>> all blocks in an if-else 'tree' should be {} if there is at least one
>> with more than two statements. (And I do not disagree.)
>
>You are actually referring to this commit:
>
>http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e659ba4a0d2d471c0d73590f78e1a1b5a1eede48
>
>which did not appear on lkml for review... :(

You know who to blame.


	Jan
-- 

Re: [PATCH] update checkpatch.pl to version 0.08

[Jan Engelhardt]

On Jul 25 2007 02:22, Paul Mundt wrote:
>Perhaps CodingStyle can start being versioned, so people can opt out of
>certain 'improvements' whenever someone has a vision, much like some
>nameless licenses.

I'd say Codingstyle is versioned by means of git commit IDs.

>
>Personally I prefer the second style, and if there's a comment block,
>then it makes sense to complete the tree with {}'s (the keyword here is
>prefer, as it's a personal preference). checkpatch has been quite useful
>for catching obviously broken things, and now it seems like it's just
>overreaching. Perhaps this functionality can be split in to a lite
>checkpatch for catching show-stoppers for application and then something
>more akin to a CodingStyle validator for the folks interested in
>arbitrarily defining convention, which they can use freely while the rest
>of us try to get something useful done.

/me thinks of ... checkpath --check-me-harder


	Jan
-- 

Re: [PATCH] update checkpatch.pl to version 0.08

[Randy Dunlap]

On Tue, 24 Jul 2007 10:20:35 -0700 Randy Dunlap wrote:

> On Tue, 24 Jul 2007 18:51:39 +0200 (CEST) Jan Engelhardt wrote:
> 
> > 
> > On Jul 24 2007 12:19, Andy Whitcroft wrote:
> > >>>> if (err) {
> > >>>>     do_something();
> > >>>>     return -ERR;
> > >>>> } else {
> > >>>>     do_somthing_else();
> > >>>> }
> > >>>
> > >>> 	if (err) {
> > >>> 		do_something();
> > >>> 		return -ERR;
> > >>> 	} else
> > >>> 		do_something_else();
> > >> 
> > >> The kool kids on linux-usb-devel largely ended up deciding that the second
> > >> version looks dorky.
> > >
> > >Ok, now if either the preceeding block or following block has {}'s then
> > >we don't report this block for being one line long.  We will miss some
> > >this way, but hey.
> > 
> > As per Ingo Molnar [ http://lkml.org/lkml/2006/9/5/68 ],
> > all blocks in an if-else 'tree' should be {} if there is at least one
> > with more than two statements. (And I do not disagree.)
> 
> You are actually referring to this commit:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e659ba4a0d2d471c0d73590f78e1a1b5a1eede48
> 
> which did not appear on lkml for review... :(

Oops, I stand corrected (my previous search was based on the patch filename):

http://lkml.org/lkml/2007/5/4/50
or
http://marc.info/?l=linux-kernel&m=117826369817272&w=2


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Randy Dunlap wrote:
> On Tue, 24 Jul 2007 10:20:35 -0700 Randy Dunlap wrote:
> 
>> On Tue, 24 Jul 2007 18:51:39 +0200 (CEST) Jan Engelhardt wrote:
>>
>>> On Jul 24 2007 12:19, Andy Whitcroft wrote:
>>>>>>> if (err) {
>>>>>>>     do_something();
>>>>>>>     return -ERR;
>>>>>>> } else {
>>>>>>>     do_somthing_else();
>>>>>>> }
>>>>>> 	if (err) {
>>>>>> 		do_something();
>>>>>> 		return -ERR;
>>>>>> 	} else
>>>>>> 		do_something_else();
>>>>> The kool kids on linux-usb-devel largely ended up deciding that the second
>>>>> version looks dorky.
>>>> Ok, now if either the preceeding block or following block has {}'s then
>>>> we don't report this block for being one line long.  We will miss some
>>>> this way, but hey.
>>> As per Ingo Molnar [ http://lkml.org/lkml/2006/9/5/68 ],
>>> all blocks in an if-else 'tree' should be {} if there is at least one
>>> with more than two statements. (And I do not disagree.)
>> You are actually referring to this commit:
>>
>> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=e659ba4a0d2d471c0d73590f78e1a1b5a1eede48
>>
>> which did not appear on lkml for review... :(
> 
> Oops, I stand corrected (my previous search was based on the patch filename):
> 
> http://lkml.org/lkml/2007/5/4/50
> or
> http://marc.info/?l=linux-kernel&m=117826369817272&w=2

Well the summary of the discussion there was one vote for the new
standard, two votes against and a "lets leave it open".  So it seems odd
it was ever added to the CodingStyle.

The "leave it undefined" makes the current behaviour of checkpatch
wrong.  So its good I've loosened it for the next release.

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Jan Engelhardt wrote:
> On Jul 25 2007 02:22, Paul Mundt wrote:
>> Perhaps CodingStyle can start being versioned, so people can opt out of
>> certain 'improvements' whenever someone has a vision, much like some
>> nameless licenses.
> 
> I'd say Codingstyle is versioned by means of git commit IDs.
> 
>> Personally I prefer the second style, and if there's a comment block,
>> then it makes sense to complete the tree with {}'s (the keyword here is
>> prefer, as it's a personal preference). checkpatch has been quite useful
>> for catching obviously broken things, and now it seems like it's just
>> overreaching. Perhaps this functionality can be split in to a lite
>> checkpatch for catching show-stoppers for application and then something
>> more akin to a CodingStyle validator for the folks interested in
>> arbitrarily defining convention, which they can use freely while the rest
>> of us try to get something useful done.
> 
> /me thinks of ... checkpath --check-me-harder

Yep I think the consensus is we need a
"--i-don't-agree-just-check-things-which-will-get-me-rejected-out-of-hand"
option of some sort which will restrict output to the real errors.

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Jan Engelhardt wrote:
> On Jul 24 2007 12:33, Andy Whitcroft wrote:
>>> Warning on multiple declarations on a line is nice, but IMO really too
>>> verbose (why is "int i, j;" bad? Did C somehow change syntax today?).
>> No the normal response is two fold:
>>
>> 1) "what the heck are i and j those are meaningless names"
> 
> Can we at least assume the submitter is sane in some ways?
> i and j are picked for obvious iterater values - you would not want
> verbosify that to fruit_iterator and process_iterator or whatever
> because it's a hell lot more typing.
> It takes more than a few Perl regexes to actually grasp the semantics
> of whether "i" is useful or not.

I was mearly quoting the what I'd seen.  I am completely ambivalent on
the whole process.  I had assumed when we updated the documentation to
strongly indicate that this was a style guide not a robot with patch
veto power that people would realise they could ignore those things they
disagreed with and things would be good.	

checkpatch is only intended to tell you what a Reviewer is likely to
pick up and winge about and is intended to save _them_ time, their time
generally being more limited that yours if for no other reason than you
want your patch in, and they may have no vested interest.

That said I want it to be as unannoying as we can and we will have
loosened most of the checks you do not like in the next release.

-apw

Re: [PATCH] update checkpatch.pl to version 0.08

[jschopp]

<snip>
 > checkpatch has been quite useful
> for catching obviously broken things, and now it seems like it's just
> overreaching. Perhaps this functionality can be split in to a lite
> checkpatch for catching show-stoppers for application and then something
> more akin to a CodingStyle validator for the folks interested in
> arbitrarily defining convention, which they can use freely while the rest
> of us try to get something useful done.

CodingStyle isn't about arbitrarily defining convention.  It is about making the codebase 
consistent, which helps a ton in readability and maintainability.

Readability is important because it makes the job of the maintainers easier.  If you or I 
have to spend 5 minutes to fix trivial CodingStyle issues, but that 5 minutes saves Andrew 
or other maintainers 60 seconds in reviewing your patch, we come out ahead.  Anything that 
shifts work from maintainers to developers is a good thing because maintainers are 
overworked as is.

It could also argue that declaring multiple variables per line or putting curly braces 
where they aren't needed doesn't make code unmaintainable.  I'd agree any one of these 
doesn't make code unmaintainable by itself.  But if all these things are added together it 
is death by 1000 cuts.  The more readable code is the fewer real bugs it will have because 
badness stands out more in clean code.

So, no we shouldn't separate out CodingStyle because

Better CodingStyle == less bugs

and

Better CodingStyle == more throughput for maintainers

Re: [PATCH] update checkpatch.pl to version 0.08

[Adrian Bunk]

On Tue, Jul 24, 2007 at 07:31:35PM +0100, Andy Whitcroft wrote:
> Jan Engelhardt wrote:
> > On Jul 25 2007 02:22, Paul Mundt wrote:
> >> Perhaps CodingStyle can start being versioned, so people can opt out of
> >> certain 'improvements' whenever someone has a vision, much like some
> >> nameless licenses.
> > 
> > I'd say Codingstyle is versioned by means of git commit IDs.
> > 
> >> Personally I prefer the second style, and if there's a comment block,
> >> then it makes sense to complete the tree with {}'s (the keyword here is
> >> prefer, as it's a personal preference). checkpatch has been quite useful
> >> for catching obviously broken things, and now it seems like it's just
> >> overreaching. Perhaps this functionality can be split in to a lite
> >> checkpatch for catching show-stoppers for application and then something
> >> more akin to a CodingStyle validator for the folks interested in
> >> arbitrarily defining convention, which they can use freely while the rest
> >> of us try to get something useful done.
> > 
> > /me thinks of ... checkpath --check-me-harder
> 
> Yep I think the consensus is we need a
> "--i-don't-agree-just-check-things-which-will-get-me-rejected-out-of-hand"
> option of some sort which will restrict output to the real errors.

No, the default should be to show only the real errors.

> -apw

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] update checkpatch.pl to version 0.08

[Adrian Bunk]

On Tue, Jul 24, 2007 at 01:45:25PM -0500, jschopp wrote:
> <snip>
> > checkpatch has been quite useful
>> for catching obviously broken things, and now it seems like it's just
>> overreaching. Perhaps this functionality can be split in to a lite
>> checkpatch for catching show-stoppers for application and then something
>> more akin to a CodingStyle validator for the folks interested in
>> arbitrarily defining convention, which they can use freely while the rest
>> of us try to get something useful done.
>
> CodingStyle isn't about arbitrarily defining convention.  It is about 
> making the codebase consistent, which helps a ton in readability and 
> maintainability.
>
> Readability is important because it makes the job of the maintainers 
> easier.  If you or I have to spend 5 minutes to fix trivial CodingStyle 
> issues, but that 5 minutes saves Andrew or other maintainers 60 seconds in 
> reviewing your patch, we come out ahead.  Anything that shifts work from 
> maintainers to developers is a good thing because maintainers are 
> overworked as is.
>
> It could also argue that declaring multiple variables per line or putting 
> curly braces where they aren't needed doesn't make code unmaintainable.  
> I'd agree any one of these doesn't make code unmaintainable by itself.  But 
> if all these things are added together it is death by 1000 cuts.  The more 
> readable code is the fewer real bugs it will have because badness stands 
> out more in clean code.
>
> So, no we shouldn't separate out CodingStyle because
>
> Better CodingStyle == less bugs
>
> and
>
> Better CodingStyle == more throughput for maintainers

To some extent yes.

But extreme codingstyling won't gain you anything.

Except for long and fruitless discussions.

If a tool says anything would be wrong with the line of C code
	int i, j;
for two loop variables, then the tool is wrong because that's an idiom 
every C programmer knows and understands.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] update checkpatch.pl to version 0.08

[jschopp]

>> Yep I think the consensus is we need a
>> "--i-don't-agree-just-check-things-which-will-get-me-rejected-out-of-hand"
>> option of some sort which will restrict output to the real errors.
> 
> No, the default should be to show only the real errors.
> 

CodingStyle violations are real errors.

If we have agreed that code should look a certain way, and there is a patch that doesn't 
look that way, that is an error.  Maybe not a runtime error, but a readability error.  A 
reviewability error.  A maintainability error.  A big waste of everybodies time.

I personally don't care if code is indented with 2 spaces, 4 spaces, or a tab.  What I do 
care about is that all the code is indented consistently so we don't waste an ounce of our 
energy reading code/patches and thinking about indentation or even worse spending our time 
arguing over it on mailing lists when there are better things to argue about.

Back when I wrote the early versions of this script I didn't write it because I'm anal 
retentive about CodingStyle.  I wrote it for the exact opposite reason.  I was tired of 
seeing email on mailing lists reviewing patches saying there was indentation with spaces 
instead of tabs, or trailing whitespace, or { on the wrong line.  It was a waste of the 
reviewers time, it was a waste of the developers time, it was a waste of the time of 
everybody on the mailing lists.  We should spend all that energy arguing over the merits 
of what the code does.

So let's argue over the CodingStyle once and be done with the argument instead of having 
the argument every day on the mailing lists forever.  We end up with more time to argue 
over much more interesting subjects and we end up with consistent code that is easy to 
read, review, and maintain.

Re: [PATCH] update checkpatch.pl to version 0.08

[jschopp]

>> So, no we shouldn't separate out CodingStyle because
>>
>> Better CodingStyle == less bugs
>>
>> and
>>
>> Better CodingStyle == more throughput for maintainers
> 
> To some extent yes.
> 
> But extreme codingstyling won't gain you anything.
> 
> Except for long and fruitless discussions.
> 
> If a tool says anything would be wrong with the line of C code
> 	int i, j;
> for two loop variables, then the tool is wrong because that's an idiom 
> every C programmer knows and understands.

I'm fine with whatever we decide is acceptable coding style, and changing the tool to 
match is work I'm willing to do.  If we decide declaring multiple variables on one line is 
bad, except if they are named i,j, or k then that's fine.  If we decide declaring 22 
variables per line is OK but 23 per line is bad then I'm fine with that.

If a check doesn't complain about bad code hundreds of times for every 1 time it complains 
about good code we will fix the check or remove the check entirely.

Andy already removed the multiple variable declaration per line check for the next version 
for that reason, it complained about arguably good code too often (to be fair many would 
say int i, j; is bad code).  Someday when we get the check fixed to handle sane multiple 
variable declarations better I'd like to add the check back in so the insane multiple 
variable declarations gets warned.

Re: [PATCH] update checkpatch.pl to version 0.08

[Adrian Bunk]

On Tue, Jul 24, 2007 at 03:32:59PM -0500, jschopp wrote:
>>> Yep I think the consensus is we need a
>>> "--i-don't-agree-just-check-things-which-will-get-me-rejected-out-of-hand"
>>> option of some sort which will restrict output to the real errors.
>> No, the default should be to show only the real errors.
>
> CodingStyle violations are real errors.
>
> If we have agreed that code should look a certain way, and there is a patch 
> that doesn't look that way, that is an error.  Maybe not a runtime error, 
> but a readability error.  A reviewability error.  A maintainability error.  
> A big waste of everybodies time.
>
> I personally don't care if code is indented with 2 spaces, 4 spaces, or a 
> tab.  What I do care about is that all the code is indented consistently so 
> we don't waste an ounce of our energy reading code/patches and thinking 
> about indentation or even worse spending our time arguing over it on 
> mailing lists when there are better things to argue about.
>
> Back when I wrote the early versions of this script I didn't write it 
> because I'm anal retentive about CodingStyle.  I wrote it for the exact 
> opposite reason.  I was tired of seeing email on mailing lists reviewing 
> patches saying there was indentation with spaces instead of tabs, or 
> trailing whitespace, or { on the wrong line.  It was a waste of the 
> reviewers time, it was a waste of the developers time, it was a waste of 
> the time of everybody on the mailing lists.  We should spend all that 
> energy arguing over the merits of what the code does.

There's a relatively small amount of common codingstyle mistakes 
accounting for most of these mistakes.

> So let's argue over the CodingStyle once and be done with the argument 
> instead of having the argument every day on the mailing lists forever.  We 
> end up with more time to argue over much more interesting subjects and we 
> end up with consistent code that is easy to read, review, and maintain.

It's also important to note that there are slightly different 
codingstyles in different parts of the kernel, and you won't get people 
to agree on one.

A common codingstyle is important, but unifying the last bits is simply 
not worth the hassle.

There are more important things than exploiting the corner cases of 
codingstyle, e.g. could you teach checkpatch.pl to give exactly two 
errors for the following code?


	while (a);
		for (b = 0; b < 50; b++);
			for (c = 0; c < sizeof(struct module); c++)
				d = e;


cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] update checkpatch.pl to version 0.08

[SL Baur]

On 7/24/07, Adrian Bunk <bunk@stusta.de> wrote:

> There are more important things than exploiting the corner cases of
> codingstyle, e.g. could you teach checkpatch.pl to give exactly two
> errors for the following code?
>
>
>         while (a);
>                 for (b = 0; b < 50; b++);
>                         for (c = 0; c < sizeof(struct module); c++)
>                                 d = e;

There are three errors there.  The while (a) busy wait needs a cpu_relax()
or something, the first for is at the wrong level of indentation and the
second for is at the wrong level of indentation relative to the first one.

Re: [PATCH] update checkpatch.pl to version 0.08

[Adrian Bunk]

On Wed, Jul 25, 2007 at 08:39:36AM -0700, SL Baur wrote:
> On 7/24/07, Adrian Bunk <bunk@stusta.de> wrote:
>
>> There are more important things than exploiting the corner cases of
>> codingstyle, e.g. could you teach checkpatch.pl to give exactly two
>> errors for the following code?
>>
>>
>>         while (a);
>>                 for (b = 0; b < 50; b++);
>>                         for (c = 0; c < sizeof(struct module); c++)
>>                                 d = e;
>
> There are three errors there.  The while (a) busy wait needs a cpu_relax()

OK, granted. But this is dummy code, and this is not what I'd expect 
checkpatch.pl to check for.

> or something, the first for is at the wrong level of indentation and the
> second for is at the wrong level of indentation relative to the first one.

If someone writes the code this way, the problem is usually not the 
indentation that is most likely correct, but the fact that the code does 
something completely different from what the author thought it would do.

We already had such bugs in the kernel, they are unlikely to be spotted 
at code review, but for a script that looks at the source code they 
should be easy to spot.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH] update checkpatch.pl to version 0.08

[Andy Whitcroft]

Jan Engelhardt wrote:
> On Jul 23 2007 16:36, Kok, Auke wrote:
>> this somehow seems to match something completely non-related (a function
>> pointer declaration case):
>>
>> ERROR: no space between function name and open parenthesis '('
>> #7278: FILE: drivers/net/e1000e/hw.h:434:
>> +       bool (*check_mng_mode)(struct e1000_hw *);
>>
>> even if I put a space between ")(", it still complains.
> 
> [assumed] majority of kernel code uses ")(" instead of ")\s+(",
> so the warning seems bogus. And it even gets nitpicky.

No that is just a false positive.  Those are meant to be detected, its
just that bool wasn't listed as a type.

-apw