Re: [PATCH] add a trivial patch style checker

[Andy Whitcroft <apw@shadowen.org>]

Andi Kleen wrote:
> Andy Whitcroft <apw@shadowen.org> writes:
>> +
>> +# no BUG() or BUG_ON()
>> +		if ($line =~ /\b(BUG|BUG_ON)\b/) {
>> +			print "Try to use WARN_ON & Recovery code rather than BUG() or BUG_ON()\n";
> 
> Just outlawing BUG_ON doesn't seem like a good idea to me. We'll just end
> up with lots of untested and likely buggy recovery code or no asserts. Both
> would be bad.

Thats not an unreasonable position.  And I tend to agree with it.
Either we try and have two levels Warnings and Errors, or we just drop
this one for now.

Anyone got an oppinion, so we can get a consensus.

>> +#need space before brace following if, while, etc
>> +		if($line=~/\(.*\){/) {
>> +			print ("need a space before the brace\n");
>> +			print "$herecurr";
>> +			$clean = 0;		
>> +		}
>> +
>> +#gotos aren't indented
> 
> You mean goto labels? Surely goto statements are to be indented.
> Confusing message

Yes, changed to labels

>> +		if($line=~/^\s*[A-Za-z\d_]+:/ and !($line=~/^\s*default:/)){
>> +			print "Gotos should not be indented\n";
>> +			print "$herecurr";
>> +			$clean = 0;
>> +		}
> 
> emacs generates one space label in front of a goto label. I wouldn't
> outlaw this.

Yep, we also now allow one space something to do with diff -p not
getting confused...

>> +# don't include <linux/video_decoder.h>
> 
> It would be probably better to define some syntax that makes it possible
> to auto extract those from feature-removal-schedule.txt. Otherwise
> long term this will become messy.

Yeah, that is a very sensible idea.

> Possible further checks that might make sense:
> - panic() anywhere in drivers/* 
> - externs in .c files without asmlinkage
> - general checking that everything in a fully visible {} block is the right 
> indentation
> 
> -Andi

Thanks.

-apw