LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Theodore Tso <tytso@mit.edu>, Adrian Bunk <bunk@kernel.org>,
	Andreas Dilger <adilger@sun.com>,
	sct@redhat.com, akpm@linux-foundation.org,
	linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [2.6 patch] fs/jbd/journal.c: cleanups
Date: Mon, 18 Feb 2008 17:22:26 +0100	[thread overview]
Message-ID: <20080218162226.GB4148@elte.hu> (raw)
In-Reply-To: <20080218151119.GC25098@mit.edu>


* Theodore Tso <tytso@mit.edu> wrote:

> On Mon, Feb 18, 2008 at 02:31:40PM +0100, Ingo Molnar wrote:
> > i guess this explains what static code metrics already suggest:
> 
> Am I right in assuming that code-quality is just a program which runs 
> checkpatch.pl and measures the number of warnings and calls them 
> errors?

no, it picks the checkpatch.pl errors and calls them errors. The script 
ignores checkpatch.pl warnings. (although even the checkpatch.pl 
warnings tend to be correct in like 90% of the cases - YMMV)

but yes, i'd agree with you if you said this was an arbitrary metric 
that does not map to the functional qualities of the code in a provable 
way.

(other than the fact that any of these style errors are valid reasons to 
reject new code that is being offered into the kernel. I.e. if the ext4 
codebase was submitted as a new, unknown filesystem right now, it would 
probably have a hard time getting accepted with all those silly style 
problems in it. Why should "old" subsystems have a lower threshold to 
meet than newer subsystems? Isnt that unfair? )

And fact is that i've yet to see really crappy code with an excellent 
checkpatch score, and really good code with a very poor checkpatch 
score. So i think you have to accept that there's _some_ correlation.

And i believe this comes from the simple fact that mental carelessness 
is contagious: lack of attention to details on the thinking level 
subsconsciously slips over into the style space as well. It's hard 
(impossible) to measure "true" code quality, but the style space is 
interrelated with the "true quality" space and is easier to measure. I 
just had a look at the errors and warnings that checkpatch --file emits 
on fs/ext3/*.c, and if i was maintaining those files i'd be fixing over 
50% of them - because they are just style inconsistencies often _within 
the same function_ which would be constant distraction when adding new 
code.

In fact, the more critical a piece of code, the more strategic it is to 
users, the less acceptable it is IMO to have "style noise" and similar 
visual distractions in it. One unusual indentation or spacing, although 
silly to even mention in isolation, _can_ trick the human eye into 
glossing over just one critical piece of information to find or avoid a 
bug. And as a bonus, consistent source code style is also an attractive 
aesthetic experience to any first-time visitor of your subsystem. BYMMV.

	Ingo

  reply	other threads:[~2008-02-18 16:23 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-02-17  8:19 Adrian Bunk
2008-02-18  7:04 ` Andreas Dilger
2008-02-18  7:12   ` Ingo Molnar
2008-02-18 11:49     ` Theodore Tso
2008-02-18 12:57       ` Ingo Molnar
2008-02-18 13:31         ` Theodore Tso
2008-02-18 13:55           ` Ingo Molnar
2008-02-18 13:12       ` Adrian Bunk
2008-02-18 13:28         ` Theodore Tso
2008-02-27 19:39           ` Adrian Bunk
2008-02-18 13:31         ` Ingo Molnar
2008-02-18 15:11           ` Theodore Tso
2008-02-18 16:22             ` Ingo Molnar [this message]
2008-02-27 21:20   ` [2.6 patch] unexport journal_update_superblock Adrian Bunk

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20080218162226.GB4148@elte.hu \
    --to=mingo@elte.hu \
    --cc=adilger@sun.com \
    --cc=akpm@linux-foundation.org \
    --cc=bunk@kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sct@redhat.com \
    --cc=tytso@mit.edu \
    --subject='Re: [2.6 patch] fs/jbd/journal.c: cleanups' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).