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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 23 Jul 2007 16:08:26 -0700
From:	"Kok, Auke" <auke-jan.h.kok@...el.com>
To:	Andy Whitcroft <apw@...dowen.org>
CC:	Andrew Morton <akpm@...l.org>, Randy Dunlap <rdunlap@...otime.net>,
	Joel Schopp <jschopp@...tin.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] update checkpatch.pl to version 0.08

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@...dowen.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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/