Reviewing Contributions

Everyone is welcome to review the contributions that are posted on Reviewboard. If you are an active gem5 user, it's a good idea to keep your eye on the contributions that are posted there (typically by subscribing to the gem5-dev mailing list) so you can speak up when you see a contribution that could impact your use of gem5. It is far more effective to contribute your opinion in a review before a patch gets committed than to complain after the patch is committed, you update your repository, and you find that your simulations no longer work.

We greatly value the efforts of reviewers to maintain gem5's code quality and consistency. However, it is important that reviews balance the desire to maintain the quality of the code in gem5 with the need to be open to accepting contributions from a broader community. People will base their desire to contribute (or continue contributing) on how they and other contributors are received. With that in mind, here are some guidelines for reviewers:

1. Remember that submitting a contribution is a generous act, and is very rarely a requirement for the person submitting it. It's always a good idea to start a review with something like "thank you for submitting this contribution". A thank-you is particularly important for new or occasional submitters.
2. Overall, the attitude of a reviewer should be "how can we take this contribution and put it to good use", not "what shortcomings in this work must the submitter address before the contribution can be considered worthy".
3. As the saying goes, "the perfect is the enemy of the good". While we don't want gem5 to deteriorate, we also don't want to bypass useful functionality or improvements simply because they are not optimal. If the optimal solution is not likely to happen, then accepting a sub-optimal solution may be preferable to having no solution. A sub-optimal solution can always be replaced by the optimal solution later. Perhaps the sub-optimal solution can be incrementally improved to reach that point.
4. When asking a submitter for additional changes, consider the cost-benefit ratio of those changes. In particular, reviewers should not discount the costs of requested changes just because the cost to the reviewer is near zero. Asking for extensive changes, particularly from someone who is not a long-time gem5 developer, may be imposing a significant burden on someone who is just trying to be helpful by submitting their code. If you as a reviewer really feel that some extensive reworking of a patch is necessary, consider volunteering to make the changes yourself.
5. Not everyone uses gem5 in the same way or has the same needs. It's easy to reject a solution due to its flaws when it solves a problem you don't have—so there's no loss to you if we end up with no solution. That's probably not an acceptable result for the person submitting the patch though. Another way to look at this point is as the flip side of the previous item: just as your cost-benefit analysis should not discount the costs to the submitter of making changes, just because the costs to you are low, it should also not discount the benefits to the submitter of accepting the submission, just because the benefits to you are low.