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

Rene Herman
2007-06-08T10:12:46Z

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.