Peer Review

Peer review can be a useful technique when programming. It ensures that at least one other person has read the code. It can catch dumb bugs and help ensure that the code is not unnecessarily obscure. Several popular programming methodologies use it. (Pair programming has the same benefits.)

Peer review has one obvious disadvantage: it slows down coding. In order for peer review to be meaningful, you have to present digestible chunks for review. And that
mean waiting for the review, or using some sort of patch management to permit continued coding until the review is complete and to incorporate changes suggested by the review.

I generally have not worked on project that require peer review. The gcc project requires maintainer approval of all changes, but maintainers are permitted to commit their own changes without review. I can see the advantages of a peer review system, provided there is some mechanism to ensure that reviews happen quickly. If reviews can linger, then projects can stall very quickly.

gcc has a difficult enough time getting patches reviewed as it is. It’s hard to recommend anything which would make it slower. One approach that might make it more acceptable would be to say that if a maintainer writes a patch, the peer review can be done by anybody–it would not have to be another maintainer. That is, require a reviewer for every patch, but only require that either the author or the reviewer be a maintainer.

I’m not sure whether this would be a good idea or not. It would be good to improve the quality of the gcc code base, but the quality is not so bad that drastic measures are required. Only a small additional cost would be acceptable.

2 Comments »

  1. Samuel Tardieu said,

    May 13, 2008 @ 11:58 pm

    About GCC: It would be great if “write after approval” maintainers could get approval from other w-a-a maintainers if the persons in charge of a subsystem don’t answer in a reasonable delay. The problem as I see it is not that patches from w-a-a maintainers are rejected by the person in charge, which is totally reasonable if there are issues with the proposed change, but that there is sometimes no feedback at all for months despite repeated “PING” requests.

    Forcing the person in charge to step up if he doesn’t want a change to go in (rather than staying mute) would probably be more productive in the case of some subsystems (the Ada front-end comes to mind). It would either increase the number of approved patches or at least improve communication and feedback.

  2. Ian Lance Taylor said,

    May 14, 2008 @ 4:51 pm

    No feedback is a huge problem. But lettimg any other write-after-approval maintainer approve the change doesn’t seem like the right answer to me. We give out write-after-approval pretty freely.

    Note that there generally isn’t a person in charge. There are several people, which makes it easier to ignore patch pings, especially for complicated patches.

RSS feed for comments on this post · TrackBack URI

You must be logged in to post a comment.