Re: share/private/slave a subtree - define vs enum

From: Roman Zippel <zippel@linux-m68k.org>
To: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Bryan Henderson <hbryan@us.ibm.com>,
	Andrew Morton <akpm@osdl.org>,
	bfields@fieldses.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, linuxram@us.ibm.com,
	mike@waychison.com, Miklos Szeredi <miklos@szeredi.hu>,
	Alexander Viro <viro@parcelfarce.linux.theplanet.co.uk>
Subject: Re: share/private/slave a subtree - define vs enum
Date: Sun, 10 Jul 2005 21:14:08 +0200 (CEST)	[thread overview]
Message-ID: <Pine.LNX.4.61.0507102047380.3728@scrub.home> (raw)
In-Reply-To: <1121019702.20821.17.camel@localhost>

Hi,

On Sun, 10 Jul 2005, Pekka Enberg wrote:

> > The point of a review is to comment on things that _need_ fixing. Less 
> > experienced hackers take this a requirement for their drivers to be 
> > included.
> 
> Hmm. So we disagree on that issue as well. I think the point of review
> is to improve code and help others conform with the existing coding
> style which is why I find it strange that you're suggesting me to limit
> my comments to a subset you agree with.
> 
> Would you please be so kind to define your criteria for things that
> "need fixing" so we could see if can reach some sort of an agreement on
> this. My list is roughly as follows:
> 
>   - Erroneous use of kernel API
>   - Bad coding style
>   - Layering violations
>   - Duplicate code
>   - Hard to read code

I don't generally disagree with that, I just think that defines are not 
part of that list.
Look, it's great that you do reviews, but please keep in mind it's the 
author who has to work with code and he has to be primarily happy with, 
so you don't have to point out every minor issue.
Although it also differs between core and driver code, we don't have to be 
that strict with driver code as longs as it "looks" ok and is otherwise 
correct. The requirements for core kernel code are higher, but even here 
defines are a well accepted language construct.

bye, Roman