[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

From: Lars Kurth <lars.kurth@xxxxxxxxxx>

This document highlights what reviewers such as maintainers and committers look
for when reviewing code. It sets expectations for code authors and provides
a framework for code reviewers.

Signed-off-by: Lars Kurth <lars.kurth@xxxxxxxxxx>
Cc: minios-devel@xxxxxxxxxxxxxxxxxxxx
Cc: xen-api@xxxxxxxxxxxxxxxxxxxx
Cc: win-pv-devel@xxxxxxxxxxxxxxxxxxxx
Cc: mirageos-devel@xxxxxxxxxxxxxxxxxxxx
Cc: committers@xxxxxxxxxxxxxx
 code-review-guide.md | 125 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100644 code-review-guide.md

diff --git a/code-review-guide.md b/code-review-guide.md
new file mode 100644
index 0000000..8639431
--- /dev/null
+++ b/code-review-guide.md
@@ -0,0 +1,125 @@
+# Code Review Guide
+This document highlights what reviewers such as maintainers and committers look
+for when reviewing your code. It sets expectations for code authors and 
+a framework for code reviewers.
+This document does **not cover** the following topics:
+* [Communication Best Practice](communication-practice.md)
+* [Resolving Disagreement](resolving-disagreement.md)
+* [Patch Submission 
+* [Managing Patch Submission with 
+## What we look for in Code Reviews
+When performing a code review, reviewers typically look for the following 
+### Is the change necessary to accomplish the goals?
+* Is it clear what the goals are?
+* Do we need to make a change, or can the goals be met with existing
+  functionality?
+### Architecture / Interface
+* Is this the best way to solve the problem?
+* Is this the right part of the code to modify?
+* Is this the right level of abstraction?
+* Is the interface general enough? Too general? Forward compatible?
+### Functionality
+* Does it do what it’s trying to do?
+* Is it doing it in the most efficient way?
+* Does it handle all the corner / error cases correctly?
+### Maintainability / Robustness
+* Is the code clear? Appropriately commented?
+* Does it duplicate another piece of code?
+* Does the code make hidden assumptions?
+* Does it introduce sections which need to be kept **in sync** with other 
+* Are there other **traps** someone modifying this code might fall into?
+**Note:** Sometimes you will work in areas which have identified 
+and/or robustness issues. In such cases, maintainers may ask you to make 
+changes, such that your submitted code does not make things worse or point you
+to other patches are already being worked on.
+### System properties
+In some areas of the code, system properties such as
+* Code size
+* Performance
+* Scalability
+* Latency
+* Complexity
+* &c
+are also important during code reviews.
+### Style
+* Comments, carriage returns, **snuggly braces**, &c
+* See 
+  and 
+* No extraneous whitespace changes
+### Documentation and testing
+* If there is pre-existing documentation in the tree, such as man pages, design
+  documents, etc. a contributor may be asked to update the documentation 
+  the change. Documentation is typically present in the
+  [docs](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) folder.
+* When adding new features that have an impact on the end-user,
+  a contributor should include an update to the
+  [SUPPORT.md](https://xenbits.xen.org/gitweb/?p=xen.git;a=tree;f=docs) file.
+  Typically, more complex features require several patch series before it is 
ready to be
+  advertised in SUPPORT.md
+* When adding new features, a contributor may be asked to provide tests or
+  ensure that existing tests pass
+#### Testing for the Xen Project Hypervisor
+Tests are typically located in one of the following directories
+* **Unit tests**: 
+  Unit testing is hard for a system like Xen and typically requires building a 
subsystem of
+  your tree. If your change can be easily unit tested, you should consider 
submitting tests
+  with your patch.
+* **Build and smoke test**: see [Xen GitLab 
+  Runs build tests for a combination of various distros and compilers against 
+  committed to staging. Developers can join as members and test their 
+  branches **before** submitting a patch.
+* **XTF tests** (microkernel-based tests): see 
+  XTF has been designed to test interactions between your software and 
+  It is a very useful tool for testing low level functionality and is executed 
as part of the
+  project's CI system. XTF can be easily executed locally on xen.git trees.
+* **osstest**: see 
+  Osstest is the Xen Projects automated test system, which tests basic Xen use 
cases on
+  a variety of different hardware. Before changes are committed, but **after** 
they have
+  been reviewed. A contributor’s changes **cannot be applied to master** 
unless the
+  tests pass this test suite. Note that XTF and other tests are also executed 
as part of
+  osstest.
+### Patch / Patch series information
+* Informative one-line changelog
+* Full changelog
+* Motivation described
+* All important technical changes mentioned
+* Changes since previous revision listed
+* Reviewed-by’s and Acked-by’s dropped if appropriate
+More information related to these items can be found in our
+[Patch submission 
+## Reviewing for Patch Authors
+The following presentation by George Dunlap, provides an excellent overview on 
+we do code reviews, specifically targeting non-maintainers.
+As a community, we would love to have more help reviewing, including from **new
+community members**. But many people
+* do not know where to start, or
+* believe that their review would not contribute much, or
+* may feel intimidated reviewing the code of more established community members
+The presentation demonstrates that you do not need to worry about any of these
+concerns. In addition, reviewing other people's patches helps you
+* write better patches and experience the code review process from the other 
+* and build more influence within the community over time
+Thus, we recommend strongly that **patch authors** read the watch the 
recording or
+read the slides:
+* [Patch Review for Non-Maintainers 
+* [Patch Review for Non-Maintainers recording - 

MirageOS-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.