Date:	Mon, 23 Feb 2009 13:45:03 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	"Rafael J. Wysocki" <rjw@...k.pl>
Cc:	Johannes Berg <johannes@...solutions.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	"Eric W. Biederman" <ebiederm@...ssion.com>,
	Benjamin Herrenschmidt <benh@...nel.crashing.org>,
	Jeremy Fitzhardinge <jeremy@...p.org>,
	pm list <linux-pm@...ts.linux-foundation.org>,
	Len Brown <lenb@...nel.org>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [RFC][PATCH 2/2] PM: Rework handling of interrupts during
	suspend-resume


* Rafael J. Wysocki <rjw@...k.pl> wrote:

> > > +resume_devices:
> > > +	resume_device_irqs();
> > 
> > Small style nit: labels should start with a space character. 
> > I.e. it should be:
> 
> I know, but the second label in there starts without a space 
> character and IMO keeping a uniform coding style i a single 
> file is more important than trying to adjust it to a broader 
> set of rules FWIW. [...]

Even though it's just a very small and insignificant detail 
(nowhere described in the CodingStyle), barely worth the mention 
(and i already regret having brought it up at all), what you say 
is wrong on a conceptual level and that alarms me a bit ;-)

It is exactly these kinds of "my code, my style!" world view 
that results in a crappy overall kernel style.

For a single file to look consistent is just the first (and 
required) step, what matters even more is for files to have 
similar coding patterns, to make the style as helpful to the 
general kernel developer/reviewer/bug-fixer/maintainer as 
possible.

"code with a helpful style" here means two things:

1) it should understand and adhere to basic style principles. 
   This is just an (often arbitrary) subset of the infinite set 
   of reasonable style guides. The most common-sense ones are
   written down in Documentation/CodingStyle. There's a lot of 
   leeway, as long as the basic principle of "be helpful" is
   understood and followed.

2) it should carry meta information outside of the language 
   syntax and it should build expectations about a code's 
   purpose and general structure.

   That is essential so that we can find bugs during review.

   If each file has a slightly different style to express labels 
   then that means we insert extra entropy and degrades and 
   obfuscates the true meat of the code and hurts the overall 
   reviewability of the code.

In practical terms: i noticed that weird label - otherwise i 
would not have commented on it. I noticed it because it had the 
pattern of a comment block (most comment blocks start with 
capital letters, and for that good reason).

It was completely unnecessary for me to notice that label - it 
carries no information about the patch itself. Ergo, it would be 
better in the long run if code does not raise unnecessary mental 
exceptions. We have a limited set of exceptions we are able to 
handle during review, lets make sure we use them sparingly.

Sure, there will always be borderline cases where we'll have to 
agree to disagree, even if we agree about the general principle.

But this is not one of those cases - having a "Suspend:" 
capitalized label is not something you added to enhance the 
basic coding style - it is something very uncommon and 
self-serving which you added in _spite_ of the general 
principles i believe. It has no other message beyond "I do this 
because i can!".

I.e. it is not helpful at all. When it comes to coding style the 
kernel is not a democracy at all.

> [...]  I also think that coding style changes shouldn't be 
> mixed with functional changes as far as reasonably possible.

Sure, you got that drive-by review for free, by virtue of 
context diffs ;-)

	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/

Powered by blists - more mailing lists