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

From: Rene Herman
Date: Fri Jun 08 2007 - 06:12:47 EST


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.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/