Re: [Xen-devel] [PATCH v4 5/7] Add Code Review Guide
On 30.12.19 20:32, Lars Kurth wrote: From: Lars Kurth 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. Changes since v3 * Added example under *Workflow from a Reviewer's Perspective* section * Fixed typos in text introduced in v2 Changes since v2 (introduced in v2) * Extend introduction * Add "Code Review Workflow" covering - "Workflow from a Reviewer's Perspective" - "Workflow from an Author's Perspective" - "Problematic Patch Reviews" * Wrap to 80 characters * Replace inline links with reference links to make wrapping easier Signed-off-by: Lars Kurth --- Cc: minios-de...@lists.xenproject.org Cc: xen-...@lists.xenproject.org Cc: win-pv-de...@lists.xenproject.org Cc: mirageos-de...@lists.xenproject.org Cc: committ...@xenproject.org --- code-review-guide.md | 313 +++ 1 file changed, 313 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 000..b2c08d2 --- /dev/null +++ b/code-review-guide.md @@ -0,0 +1,313 @@ +# 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 provides +a framework for code reviewers. + +Before we start, it is important to remember that the primary purpose of a duplicate "a" +a code review is to identify any bugs or potential bugs in the code. Most code +reviews are relatively straight-forward and do not require re-writing the +submitted code substantially. + +The document provides advice on how to structure larger patch series and +provides pointers on code author's and reviewer's workflows. + +Sometimes it happens that a submitted patch series made wrong assumptions or has +a flawed design or architecture. This can be frustrating for contributors and +code reviewers. Note that this document does contain [a section](#problems) +that provides suggestions on how to minimize the impact for most stake-holders +and also on how to avoid such situations. + +This document does **not cover** the following topics: +* [Communication Best Practice][1] +* [Resolving Disagreement][2] +* [Patch Submission Workflow][3] +* [Managing Patch Submission with Git][4] + +## What we look for in Code Reviews + +When performing a code review, reviewers typically look for the following things + +### 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 + sections? +* Are there other **traps** someone modifying this code might fall into? + +**Note:** Sometimes you will work in areas which have identified maintainability +and/or robustness issues. In such cases, maintainers may ask you to make +additional 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 +* +are also important during code reviews. + +### Style + +* Comments, carriage returns, **snuggly braces**, +* See [CODING_STYLE][5] and [tools/libxl/CODING_STYLE][6] +* 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 + alongside the change. Documentation is typically present in the [docs][7] + folder. +* When adding new features that have an impact on the end-user, + a contributor should include an update to the [SUPPORT.md][8] 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**: [tools/tests][9] or [xen/test][A] + Unit testing is hard for a system like Xen and typically
[Xen-devel] [PATCH v4 5/7] Add Code Review Guide
From: Lars Kurth 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. Changes since v3 * Added example under *Workflow from a Reviewer's Perspective* section * Fixed typos in text introduced in v2 Changes since v2 (introduced in v2) * Extend introduction * Add "Code Review Workflow" covering - "Workflow from a Reviewer's Perspective" - "Workflow from an Author's Perspective" - "Problematic Patch Reviews" * Wrap to 80 characters * Replace inline links with reference links to make wrapping easier Signed-off-by: Lars Kurth --- Cc: minios-de...@lists.xenproject.org Cc: xen-...@lists.xenproject.org Cc: win-pv-de...@lists.xenproject.org Cc: mirageos-de...@lists.xenproject.org Cc: committ...@xenproject.org --- code-review-guide.md | 313 +++ 1 file changed, 313 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 000..b2c08d2 --- /dev/null +++ b/code-review-guide.md @@ -0,0 +1,313 @@ +# 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 provides +a framework for code reviewers. + +Before we start, it is important to remember that the primary purpose of a +a code review is to identify any bugs or potential bugs in the code. Most code +reviews are relatively straight-forward and do not require re-writing the +submitted code substantially. + +The document provides advice on how to structure larger patch series and +provides pointers on code author's and reviewer's workflows. + +Sometimes it happens that a submitted patch series made wrong assumptions or has +a flawed design or architecture. This can be frustrating for contributors and +code reviewers. Note that this document does contain [a section](#problems) +that provides suggestions on how to minimize the impact for most stake-holders +and also on how to avoid such situations. + +This document does **not cover** the following topics: +* [Communication Best Practice][1] +* [Resolving Disagreement][2] +* [Patch Submission Workflow][3] +* [Managing Patch Submission with Git][4] + +## What we look for in Code Reviews + +When performing a code review, reviewers typically look for the following things + +### 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 ef???cient 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 + sections? +* Are there other **traps** someone modifying this code might fall into? + +**Note:** Sometimes you will work in areas which have identified maintainability +and/or robustness issues. In such cases, maintainers may ask you to make +additional 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 +* +are also important during code reviews. + +### Style + +* Comments, carriage returns, **snuggly braces**, +* See [CODING_STYLE][5] and [tools/libxl/CODING_STYLE][6] +* 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 + alongside the change. Documentation is typically present in the [docs][7] + folder. +* When adding new features that have an impact on the end-user, + a contributor should include an update to the [SUPPORT.md][8] 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**: [tools/tests][9] or [xen/test][A] + Unit testing is hard for a system like Xen and typically requires building a + subsystem of your tree. If your
