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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
