reviewboard_guidelines
A PCRE internal error occured. This might be caused by a faulty plugin
====== Differences ====== This shows you the differences between two versions of the page.
| — |
reviewboard_guidelines [2014/03/30 15:14] (current) powerjg created |
||
|---|---|---|---|
| Line 1: | Line 1: | ||
| + | ====== Patch review guidelines ====== | ||
| + | |||
| + | ===== Steps to submit a patch review ===== | ||
| + | |||
| + | - Create a patch.\\ Use mercurial queues to create a patch which can be applied to the **most recent** development branch of gem5-gpu. | ||
| + | - Be sure the patch adheres to the gem5 style guideline.\\ The easiest way to do this is to add the gem5 style hook to your .hg/hgrc. | ||
| + | - Run the regression tests.\\ Unless there is a good reason (with an explanation) statistics should not change. | ||
| + | - Upload patch to reviewboard.\\ Use hg postreview to post the patch. | ||
| + | - On reviewboard, either state that the patch passes the regression tests with no changes, or explain the changes to the regressions. | ||
| + | - Wait for reviews and then address any opened issues. | ||
| + | - Your code can now be added to gem5-gpu! | ||
| + | |||
| + | ===== Guidelines for reviewers ===== | ||
| + | |||
| + | When reviewing a patches, reviewers should strive to be as specific as possible. This includes both specifically pointing out the problem and clearly communicating what is required to resolve the issue. | ||
| + | Below is a set of specific reasons that a patch can be blocked. In the absence of these conditions, the patch should be committed to the mainline gem5-gpu repository. | ||
| + | |||
| + | ==== Reasons to hold-up a patch ==== | ||
| + | |||
| + | Reviewers may open issues for the following reasons, which must be resolved by the submitter. | ||
| + | * Patches should not have bugs!\\ If there are any obvious bugs from reading through the code it's incredibly helpful if we catch them before they are pushed into the mainline.\\ Note: The onus is on the patch submitter to make sure that things don't break (i.e. pass the regressions). However, mistakes happen, and if bugs are present in a already applied changeset a new review request should be submitted to fix the problem. | ||
| + | * Patches should pass the regression tests. If statistics change or are added, there should be a brief explanation for the changes. | ||
| + | * Patches should have sufficient comments. This includes (but is not limited to) comments on each non-obvious (more than about 5 lines) function, comments on each class, and comments in complex functions explaining the code. | ||
| + | * Patches should adhere to the gem5 style guidelines. We strive for readable code. These guidelines are a good baseline for that. | ||
| + | * Patches should be small and to the point. \\ As examples, patches should touch few modules and should not insert any unnecessary/dead code. When possible, please try to avoid changes outside of gem5-gpu (i.e. in gem5 or gpgpu-sim) and avoid changes to libcuda that will require users to recompile benchmarks. | ||
| + | * Patches should not remove existing functionality. | ||
| + | |||
| + | Obviously each of these can have exceptions where a patch can be pushed into mainline gem5-gpu without meeting a specific requirement, but exceptions should be just that: exceptional. | ||
| + | Similarly, there are exceptional reasons to hold up a patch for something other than the above (including the guidelines below in rare cases). In these cases, the onus is on the patch reviewer to clearly express why such an exception should be made. | ||
| + | |||
| + | ==== Other code guidelines ==== | ||
| + | |||
| + | Reviewers are allowed to request the below changes, but strict adherence is optional for patches. These are strongly recommended for promoting maintainable and extensible code. | ||
| + | * Patches should have extensible object-oriented design.\\ This includes appropriate use of inheritance and encapsulation, the use of minimal data structures, etc. | ||
| + | * Component microarchitectures should either explicitly model all pipeline stages or incur appropriate latencies when using functional modeling. | ||
| + | * New features should come with a regression test for the feature. | ||
reviewboard_guidelines.txt ยท Last modified: 2014/03/30 15:14 by powerjg
Page Tools
Except where otherwise noted, content on this wiki is licensed under the following license: CC Attribution-Share Alike 3.0 Unported
