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

[Vojtech Pavlik <vojtech@suse.cz>]

On Sun, Jul 10, 2005 at 09:21:42PM +0300, Pekka Enberg wrote:

> 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
 
The reason people post their patches for review is to get good feedback
on them. The problems you list above are mostly nitpicks. They must be
fixed before inclusion of the patch, but only make sense to start fixing
once the patch does a reasonable change.

Often patches have deeper problems (like "this won't ever work", "there
is a nice race hidden in there", "why do we need this part at all"), and
spotting those is much more valuable for both the sumbitter and the
progress of development.

Obviously, it's much harder to do that than to comment on a misplaced
brace.

It's an utter waste of effort to force a first time patch author to fix
all the style issues in his patch, just to see it rejected by the
maintainer because it is fundamentally wrong later.

Just something to consider.

-- 
Vojtech Pavlik
SuSE Labs, SuSE CR
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/