[PATCH] Fix dominator descendants for unreachable blocks.

[Chandler Carruth]

On Fri, Nov 29, 2013 at 9:47 AM, Renato Golin <renato.golin at linaro.org>wrote:

> But since this patch is really obvious, and because I'm sure you ran
> all the tests, you should be able to commit if no one else reply to it
> in a day or so, and we can do post-commit review.
>

Totally off the topic of *this* patch, but I wanted to reply directly to
this comment.

I'm somewhat firmly opposed to this practice, and have said so repeatedly
on the list. Once someone has decided to mail out a patch for pre-commit
review, it was insufficiently obvious *to them* for post-commit review.
Stick with the review, and get an LGTM. Trivial patches are fast to review,
and so it is exceedingly rare that these actually cause the big bottlenecks
in the community's review process.

Renato, the obviousness of the patch to you may make you comfortable giving
an LGTM outside of your area, and that seems totally fine (although note
that I spotted a bug in the patch). However, I don't ever see a reason for
"if no one else replies in a day or so" or "wait a few days and then go
ahead"[1]. The fact that a reviewer (or author) feels the need to wait is
an indication that they aren't confident in their review and thus someone
else should provide the final review. This doesn't mean that this kind of
early review isn't useful -- it totally is!!! A decent chunk of the time it
can spot obvious goofs quickly and early, and let the author update for
them. It can reduce the cycles of review when a person who knows the area
shows up and can look at it. It also helps build the reviewer's experience
in the area (and this is exactly how a i learned several areas of LLVM and
Clang!) so don't take this as discouraging these reviews at all. They are
awesome, thank you for doing them and keep doing them! =]

It's just that ultimately, unlike wine, patches don't get better with age.
;]

Anyways, I'll get off my soapbox and go do something useful.
-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131129/9c3e65d6/attachment.html>