lkml.org 
[lkml]   [2008]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: Merging of completely unreviewed drivers

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> I'm personally of the opinion that a lot of checkpatch "fixes" are
> anything but. That mainly concerns fixing overlong lines (where the
> "fixed" version is usually worse than the original), but it's been
> true for some other warnings too.

that was certainly the case for the earlier checkpatch releases which
treated overlong lines as an error.

So here's a quick list of negative and positive aspects of current
versions of checkpatch, as i see them.

But let me first declare it that when scripts/checkpatch.pl was
initially merged last year i immediately ran it over my own files and
became a deep sceptic of it. (check the lkml archives, i complained alot
about it)

Now i've got more than half a year of experience with using checkpatch
as an integral part of scheduler maintenance, and we've now got 4 months
of experience with using checkpatch in arch/x86 maintenance.

Based on this first hand experience, my opinion about checkpatch has
changed, rather radically: i now believe that checkpatch is almost as
important to the long term health of our kernel development process as
BitKeeper/Git turned out to be. If i had to stop using it today, it
would be almost as bad of a step backwards to me as if we had to migrate
the kernel source code control to CVS.

Lets see the Bad Side of checkpatch:

1) checkpatch "errors" shouldnt be taken too seriously for newly
introduced "leaf" driver code, which code we dont at all know
whether we'll be maintaining in any serious manner in the future.
Slowing down a submission by requirig it to pass checkpatch is not
as clear-cut as it is for core infrastructure and architecture code.
It's far more important to get _any_ code to users (as long as it's
not outright harmful) than to nitpick about style details.

2) it still has some false positives. (They are quite rare in the
latest versions, about 1 out of 100 for code that is already
"clean". I send them over to Andy whenever i see them, and they get
fixed quickly. The false positives were a big annoyance in early
checkpatch.pl versions, these days they are not - to me at least.)

3) it's _really_ annoying when sometimes i stumble over some old,
crufty piece of code that according to checkpatch is in high need of
some good, thorough cleanup - and when i take a look at the code it
turns out that the original author of that crap piece of code turns
out to be ... me. Those moments can be pretty embarrasing and
sobering ;-)

The Good Side of checkpatch (and here i'll only list the non-obvious
advantages):

1) 90% of the scheduler related checkpatch fixes today you'll never
recognize in a commit! The fixes all happen before code is
submitted, and the fixes are seemlessly embedded in nice looking
patches. (in that sense checkpatch is a bit like lockdep: 90% of the
errors they detect wont hit lkml, ever.)

2) you might know that Deja-Vu moment when you look at a new patch that
has been submitted to lkml and you have a strange, weird "feeling"
that there's something wrong about the patch.
It's totally subconscious, and you take a closer look and a few
seconds later you find a real bug in the code.

That "feeling" i believe comes from a fundamental property of how
human vision is connected to the human brain: pattern matching.
Really good programmers have built a "library" of patterns of "good"
and "bad" looking coding practices.
If a patch or if a file has a clean _style_, bugs and deeper
structural problems often stand out like a sore thumb. But if the
code is peppered with random style noise, it's a lot harder (for me
at least) to notice real bugs. I can notice bugs in a squeeky clean
code base about 5 times easier than in a noisy codebase. This effect
alone makes checkpatch indispensible for the scheduler and for
arch/x86.

Sidenote: i dont really need fancy metrics trying to tell me how
good an algorithm _truly_ is (although it certainly would be
interesting to have). I can _see_ that at a glance - provided the
code follows common kernel practices and a common, consistent style.
Checkpatch makes visual code patterns universal and eases the human
maintainance work enormously, for a 150+ KLOC subsystem like
arch/x86. I'm not distracted (visually and mentally) by the thick
fog of small silly details and quirks in coding style. Others might
have radar eyes and radar brains, i dont :-)

3) checkpatch also keeps _my_ bugs out of the kernel in an interesting
way. I'm sure many of you are like me: i've got "weaker" moments
when i write rather crappy code, and i've got "stronger" moments
when i'm in the flow and can write a few thousand lines of code with
nary a hickup. What makes things worse is it's really hard to tell
the two apart.

It turns out - and this surprised me a lot - that when i write new
code that is "weaker", i tend to make more "style mistakes", without
noticing them. Later on, when i do a checkpatch run, i see some
weird looking code and find that it's also buggy!

This concept also works with code written by others: when i get a
careless patch written in a hurry, it is much more likely to have
style errors in it, and as a maintainer i'm warned about that fact.

The best programmers are the ones who have a good eye for details -
and that subconsciously extends to "style details" too. I've yet to
see a _single_ example of a good, experienced kernel programmer who
writes code that looks absolutely careless and sloppy, but which is
top-notch otherwise. (Newbies will make style mistakes a lot more
often - and for them checkpatch is a nice and easy experience at
reading other people's code and trying to learn the style of the
kernel.)

4) there's a psychological effect as well: clean _looking_ code is
more attractive to coders to improve upon. Once the code _looks_
clean (mechanically), the people with the real structural cleanups
are not far away either. Code that just looks nice is simply more of
a pleasure to work with and to improve, so there's a strong
psychological relationship between the "small, seemingly unimportant
details" cleanups and the real, structural cleanups.

On the other hand, bad looking, unaesthetic code is avoided by
kernel developers like the pest. That is a constant skewing force
that is very harmful to Linux, because the "current style" of
subsystems is a pretty random property at the moment, and there are
_many_ important codebases in the kernel that are avoided by most of
us purely just because they look so awful.

5) cleanups were rather hard to get upstream before, because there was
never any true "objective basis" for the cleanups, giving an easy
excuse for flames over stupid taste differences, and making it easy
for maintainers to reject 90%-good cleanups just based on taste
differences. Checkpatch gives the right tool to people to write
consistently clean code and makes it harder for maintainers to find
the arguments to keep keep code unclean.

After this list of rather subjective impressions, i've also got some
historic raw data as well about how arch/x86 cleanups progressed over
the past 4 months.

( NOTE: the "errors" count below does _not_ include "lines longer than
80 chars" warnings nor any of the other checkpatch warnings - only
checkpatch "errors" which are real bona fide style errors in 99%+ of
the cases. )

errors lines of code errors/KLOC
........................................................................
v2.6.24-rc1 arch/x86/ [23 Oct 2007] 8695 117423 74.0
v2.6.24-x86.git arch/x86/ [21 Nov 2007] 5190 117156 44.2
v2.6.24-x86.git arch/x86/ [18 Dec 2007] 4057 117213 34.6
v2.6.24-x86.git arch/x86/ [ 8 Jan 2008] 3650 117987 30.9
v2.6.24-x86.git arch/x86/ [ 4 Feb 2008] 3334 133542 24.9
v2.6.25-x86.git arch/x86/ [21 Feb 2008] 2724 136963 19.8
[ See: http://redhat.com/~mingo/x86.git/code-quality - although i guess
i should rename it to "style-quality" - because there is no direct
mapping of style quality to real code quality. NOTE: some of the
reductions in the error count above are mechanic from things like
really long arrays or the math-emu changes - but most of the real
reductions are genuine. ]

v2.6.24-rc1 was the raw arch/x86 code how we inherited it after we did
the mechanic unification without changing any of the files. After that
point you can see a marked reduction in the total count of style errors.

While many of the fixes are just small details and may all seem
insignificant in isolation, IMO the sum of those small details matters
_a lot_: in the past 4 months the code has become a lot more hackable to
us and that process was driven in large part by checkpatch.

Ingo


\
 
 \ /
  Last update: 2008-02-22 19:57    [from the cache]
©2003-2011 Jasper Spaans