Code reviews

Code submissions to ns-3-dev are expected to go through a review process where one or more maintainers comment on the code. Contributors may be asked to revise their code or rewrite portions during this process. New features are also typically only merged during the beginning of a new release cycle, to avoid destabilizing code before release.

ns-3 reviews are conducted on the ns-3-reviews Google group and on the ns-developers mailing list for reviews that involve code that cuts across maintainer boundaries or is otherwise controversial.

The basic purpose of the code reviews is to ensure the long-term maintenance and overall system integrity of ns-3. Going through detailed code reviews is a departure in practice from ns-2, where many contributed modules were integrated without style or consistency requirements. For ns-3, we feel that code reviews are essential to maintaining a high quality simulation tool and reducing the potential for simulation errors.

Some basic principles of the ns-3 reviews are to enforce the coding style requirements and guidelines and to review for consistency in overall system design and performance. A bad outcome (to be avoided) is when the contributor is asked to refactor code that is otherwise stylistically compliant late in the design/review process. Sometimes the contributor may feel annoyed at somewhat arbitrary application of rules. In such a situation, it is best to discuss points in question on the public mailing lists. For aspects of the contribution that are self contained and don't affect the rest of the simulator, when one of the (non-mandatory) coding style or engineering guidelines is perceived to be not followed, maintainers/reviewers should offer suggestions such as here is the ns-3 way to do that or here is how I recommend you should change it but leave it up to the contributor how he or she wants to deal with the comment. Contributors should respond somehow to review suggestions not followed, even if it is a response like hmm, that is simply too much work to do for the benefit.

Maintainers should be free to ask contributors to rewrite proposed modifications to existing classes in the codebase, such as base classes. Because this is a bad outcome for the contributor, the maintainer should try to avoid this, but if deemed necessary, offer technical rationale why and be willing to debate this on the list (sometimes the maintainer is wrong). If the maintainer is ultimately not convinced, the contributor should follow the maintainer's guidelines. Again, to avoid surprises, contributors are encouraged to contact maintainers early in the process if they are planning to modify existing ns-3 code or APIs.

One final important issue that we touched on briefly above is system integrity. What we have seen in the past from ns-2 is that people contribute models that have been tested within a specific configuration range (e.g. running on a WiFi network only), but may not do something sane with respect to other possible configurations. If we do not look at these overall system issues and require that, at the very least, contributed models do something reasonable when they are configured outside their range of applicability (even if the behavior is just a hard assert), invariably there will be user mail in the future to the effect of I tried to use model X with configuration Y and got a surprising outcome. That is if we are lucky; the interaction may be error prone but not reveal any warning signs to the user. So, maintainers and reviewers may require contributors to deal with these (current and potential) system issues.

More information about the primary tool, Rietveld, which the project uses for code reviews, can be found here.