[ left on tech-kern, because I think it's a more general statement
  than just for this thread or these individuals ]

On Sat, Mar 17, 2007 at 09:49:22AM +0100, Juan RP wrote:
> On Saturday 17 March 2007, YAMAMOTO Takashi wrote:
> > i reviewed your code and pointed some problems because i thought
> > it would help you develop the code.  sorry if it merely annoyed you.
> 
> No problem, but why don't you try to help me rather than saying
> "you are not qualified to commit this code because you are not
> understanding how it works?" if you say exactly what's wrong or
> how to improve it, I'll fix.

In this case, the issues seem to be solved, but I would still like
to say that yamt is serving a useful function in reviewing the
code, and I would hate to discourage that.  It is sometimes clear
that there is a problem, even if the solution is not clear.  I don't
think yamt knew off-hand exactly why things weren't working with
a single pass or how to fix the issue, but he recognized it as a
workaround.

A code review is a chance to learn where the weaknesses are in
the code.  The reviewer may not have all of the answers.  The
person putting code up for review should be ready to answer
questions about why things were done in a certain way, even if
the answer is itself a question, "I don't know.  That's the only
way I could get it to work.  Do you know a better way?"  I think
it's entirely valid for the code reviewer to say, "It looks wrong,
could you take another look at it, please, to see if you missed
something?"  Or "No, I don't.  But xxx@ or yyy@ might have some
other ideas."

The wierder the code, the more effort should go into understanding
it.  Two loops worked in your system--if you don't know why, how
do you know it'll work in other systems (perhaps with more CPUs
or CPUs from another generation or vendor)?

> I'm happy learning every day more things about NetBSD, but if you
> don't say what's wrong it's harder to understand how the code works!

Sometimes experience can suggest that something is wrong without
knowing exactly why it's wrong.  If that's the case, then the usual
answer is either that there really is something wrong with the
approach that just hasn't been identified or that there's some kind
of underlying hardware issue or algorithmic issue that the code is
working around.  In these last cases, the question becomes whether
or not you understand the reasons behind the behavior.  I think
that in this case, yamt's concern was that you had a workaround,
but not an understanding of what you were working around.  That's
a recipe for hard-to-understand bugs in the future.  Or even mistaken
assumptions that will be (mis)applied in the future.

I know it's hard when you submit code that you've worked on, that
you know is useful, and that you'd like to get out to other people,
but when you submit it, you get seemingly vague response.  In this
community, we have an added difficulty in communication sometimes
because we are unable to all use our native languages.

Perhaps we can all work harder to:

	a) Try to be a little more explicit in responses about why
	   we raise issues and the limit of our specific knowledge,
	   and

	b) Try to be a little more humble and inquisitive about
	   reviewers' comments.  If a reviewer is vague, try to
	   understand the reason for the vagueness.  What is at
	   the root of the concern?

In short, let's work together cooperatively.  I, for one, appreciate
seeing active development, active review of changes, and improvement
of the code that goes into the system.

Thanks,
-allen

-- 
Allen Briggs  |  http://www.ninthwonder.com/~briggs/  |  briggs@ninthwonder.com