On Mon, Jun 27, 2016 at 12:43:32PM -0700, Stefan Beller wrote:
> There are a few issues:
>
> 1) How did you spot the bug? ("Experience/Logical thinking" is the hand
> wavy answer, but if you had a list like
> [ ] check for mem leak
> [ ] check for futureproof design
> [ ] check for failure modes (What if a syscall fails?")
> [ ] ... List is not complete, but has some made up points
I use the hand-wavy approach to reviews, and I'm pretty sure that's how
I saw your bug (just thinking about what the code would do, how I would
write it, and then noticing a discrepancy).
I suspect a checklist could make things more thorough, but I think it
can also discourage deep thinking. Ideally we have some reviewers who
are checklist-oriented looking for those sorts of details and some who
are not.
> 2) I used to send out only "done"s, i.e. when I already fixed the
> issue, instead of acknowledging the problem and postponing the fix
> for later. I'll revert to that.
Yes, I tend to use the review thread as a todo list, and do a single
session where I read over all of the review mails again (even if I've
read and responded to them already), fixing or adjusting the code. Sort
of a human version of the automated tool I described. :)
> > Of course, none of that would have helped my comment, which was in a
> > "PS" several emails deep in a discussion thread. ;)
>
> Maybe a "P.S." would get its own point in the todo list not associated
> with code? Then it would still be on the radar.
Yes, there are definitely design points that are not about a specific
part of the code, and you would want to somehow represent them in your
todo list.
There is a danger, though, of having a bunch of false positives added to
your list: tangent discussion that distracts you from the actual review.
And I think the bug here being in a PS is less relevant than being
buried amidst other discussion. That made it hard for a tool _or_ a
human to find.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html