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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Tue, 24 Jul 2007 13:47:13 +0200
From:	Ingo Molnar <mingo@...e.hu>
To:	Andy Whitcroft <apw@...dowen.org>
Cc:	"Kok, Auke" <auke-jan.h.kok@...el.com>,
	Andrew Morton <akpm@...l.org>,
	Randy Dunlap <rdunlap@...otime.net>,
	Joel Schopp <jschopp@...tin.ibm.com>,
	linux-kernel@...r.kernel.org,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] update checkpatch.pl to version 0.08

* Andy Whitcroft <apw@...dowen.org> wrote:

> > within the last 3 weeks, this script went from *really usable* to *a 
> > big noise maker*.

seconded ...

v0.06 was "almost there". I fixed kernel/sched.c to be completely clean, 
except for 3 false positives. That was a real improvement, and i started 
to like checkpatch.pl.

v0.08 is a clear step backwards: it emits 61 warnings now, 90% of which 
are totally bogus. The only 'fix' for many of those warnings is to make 
the code _worse_. That is unacceptable.

> > Warning on multiple declarations on a line is nice, but IMO really 
> > too verbose (why is "int i, j;" bad? Did C somehow change syntax 
> > today?).
> 
> No the normal response is two fold:
> 
> 1) "what the heck are i and j those are meaningless names"
> 2) "please can we have some comments for those variables"

you really should not even be arguing about this. LOOK AT the many false 
positives in sched.c. This is perfectly readable code:

 void __init sched_init(void)
 {
         u64 now = sched_clock();
         int highest_cpu = 0;
         int i, j;

         for_each_possible_cpu(i) {
                 struct rt_prio_array *array;
                 struct rq *rq;

                 rq = cpu_rq(i);

this warning for "i, j" is clearly bogus. So are many of the other 
warnings. checkpatch.pl went from a useful tool that improved the 
quality of the kernel to a rigid, unflexible policeman. It needs to be 
fixed or needs to be gotten rid of.

> which normally leads to the suggestion that it be the following form:
> 
> 	int source;		/* source clock hand */
> 	int destination;	/* destination clock hand */

what the hell are you thinking? Not every trivial line of code needs to 
be commented. Comments are needed for the _nontrivial_ lines of code, 
and there's no way a tool can decide that. The longer "destination" 
variable suggested by you can _easily_ make a previously readable piece 
of code unreadable.

	Ingo
-
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/