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

[Rene Herman <rene.herman@gmail.com>]

On 06/08/2007 11:31 AM, Andy Whitcroft wrote:

> Ok, the latest version 0.04 is released and I have also put up the
> complete script at:
> 
>   http://www.shadowen.org/~apw/public/checkpatch/checkpatch.pl-0.04
> 
> Previous versions are also there.

Thank you. False positive:

	do not use assignment in condition
	#809: FILE: drivers/cdrom/mitsumi.c:766:
	+       while ((req = elv_next_request(q))) {

Doing an assignment in a while (or for) condition like that makes perfect 
sense. The check should probably be limited to if conditions -- you can 
always split those.

Next:

	struct mutex definition without comment
	#173: FILE: drivers/cdrom/mitsumi.c:130:
	+	struct mutex mutex;

Going overboard. It's the only locking primitive in the driver; the only 
comment that could be added is something like "used for mutual exclusion" 
which firmly falls into the "i++; /* increase i */" category. A bit further 
on up in the driver, a:

	/*
	 * LOCKING: mutex_lock(&mcd->mutex)
	 */

is present just before the functions that need to be called with it held 
(all, basically). I'd suggest simply dropping this check as its intention is 
not something that can be usefully automated I feel. The tree would end up 
with tons of useless comments that are there just to shut up the script.

And next a ton of:

	labels should not be indented
	#249: FILE: drivers/cdrom/mitsumi.c:206:
	+  out:

This driver is putting two spaces in front of labels, as explained here:

	http://lkml.org/lkml/2007/6/5/61

I do agree that putting them level aligned is a _significantly_ different 
style, so perhaps it wants to warn if a label is not within the first indent 
level (column 0-7) but if even that's contentious then this check should 
perhaps go completely as well. One or two spaces, four for all I care, can 
be considered a personal preference I feel.

Rene.