Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Hi Lars, On 18/12/2019 17:09, Lars Kurth wrote: On 18/12/2019, 14:29, "Julien Grall" wrote: Hi Lars, On 12/12/2019 21:14, Lars Kurth wrote: > +### Workflow from an Author's Perspective > + > +When code authors receive feedback on their patches, they typically first try > +to clarify feedback they do not understand. For smaller patches or patch series > +it makes sense to wait until receiving feedback on the entire series before > +sending out a new version addressing the changes. For larger series, it may > +make sense to send out a new revision earlier. > + > +As a reviewer, you need some system that he;ps ensure that you address all Just a small typo: I think you meant "helps" rather than "he;ps". Cheers, Thank you: fixed in my working copy. One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of patch. Normally the ACKed-by or Reviewed-by is a signal that it is > I am assuming it is, but I think it may be worthwhile pointing this out in the document, that unless stated otherwise, the reviewer is happy with the patch I don't think you can always assume this. There are case where I don't give a reviewed-by yet because I want to understand the follow-up patches first. I think what Ian described correspond the best to my view here. And I agree that we probably want to be more explicit in the review to avoid confusion. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Ian Jackson writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> > In a nutshell, in such a review the possible interpretations are
> > * I reviewed, but didn't feel qualified to do the rest
> > * I reviewed, but did not get round to give it full attention
> > * I reviewed, but before I make a final decision want to look at the next
> > version
> > ...
> > * I reviewed and don't object the rest
> > * I reviewed and agreed with the rest
> > The latter two are equivalent to Ack/R-b
> >
> > That is quite a large range of possibilities and kind of leaves the author
> > guessing what state the review is in
...
> IOW: if you do not get an A-b or R-b, then the person writing is not
> necessarily making any of the statements you posit which start "I
> reviewed".
The other point it occurs to me to make here is that whether someone
reviewed something is not of formal importance to the process,
although it is of course very relevant information.
With my maintainer hat on I frequently approve patches which I have
not reviewed.
Likewise I sometimes read and even review in detail patches where I
have no formal authority. My comments are then intended (in a formal
sense) as input to the maintainers' decisionmaking.
(In practice, since we are all working together to make the best
software, maintainers generally expect a submitter to address issues
raised by a review from a non-maintainer, and strong objections from
non-maintainer stakeholders usually lead to a discussion about how to
resolve matters. This is all cooperation and common courtesy I
think.)
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Lars Kurth writes ("Re: [PATCH v3 5/7] Add Code Review Guide"):
> In a nutshell, in such a review the possible interpretations are
> * I reviewed, but didn't feel qualified to do the rest
> * I reviewed, but did not get round to give it full attention
> * I reviewed, but before I make a final decision want to look at the next
> version
> ...
> * I reviewed and don't object the rest
> * I reviewed and agreed with the rest
> The latter two are equivalent to Ack/R-b
>
> That is quite a large range of possibilities and kind of leaves the author
> guessing what state the review is in
There are only three possibilities:
Acked-by:
I hereby bless this with my maintainer powers.
I may or may not have reviewed it. The body text may contain
more information about that.
Reviewed-by:
I have reviewed this to my own satisfaction and this mail contains
all the comments I have on it. If I am a maintainer, I hereby
bless it with my maintainer powers. (This is a weird use of the
word "Reviewed" since in usual usage one can review something and
end up disapproving of it; nevertheless this is the convention.)
In both of the above cases, additionally
If my approval is conditional (eg on changes) this will be
stated explicitly in the body text of my message.
Neither of the above:
I have read at least some of this and felt motivated to make some
observations. If I have reviewed it properly this would usually be
stated in the body text. If I am taking enough of an interest in
this patch, the body text may also say what I think the current
state and/or next steps should be. In any case, I do *not* bless
this patch (in its current form) with any maintainer powers I may
have.
IOW: if you do not get an A-b or R-b, then the person writing is not
necessarily making any of the statements you posit which start "I
reviewed".
Having said that it is a good idea for people commenting on patches to
make clear what they have and haven't done. I often start a message
with "Thanks for this patch which I have reviewed. I have some
comments" or words to that effect.
Ian.
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
On 19.12.2019 11:03, Lars Kurth wrote: > > >> On 19 Dec 2019, at 09:56, Jan Beulich wrote: >> >> On 18.12.2019 18:09, Lars Kurth wrote: >>> >>> >>> On 18/12/2019, 14:29, "Julien Grall" wrote: >>> >>>Hi Lars, >>> >>>On 12/12/2019 21:14, Lars Kurth wrote: +### Workflow from an Author's Perspective + +When code authors receive feedback on their patches, they typically first try +to clarify feedback they do not understand. For smaller patches or patch series +it makes sense to wait until receiving feedback on the entire series before +sending out a new version addressing the changes. For larger series, it may +make sense to send out a new revision earlier. + +As a reviewer, you need some system that he;ps ensure that you address all >>> >>>Just a small typo: I think you meant "helps" rather than "he;ps". >>> >>>Cheers, >>> >>> Thank you: fixed in my working copy. >>> >>> One thing which occurred to me for reviews like these, where there is no >>> ACK's or Reviewed-by's is that I don't actually know whether you as >>> reviewer is otherwise happy with the remainder of the patch. >>> Normally the ACKed-by or Reviewed-by is a signal that it is >>> >>> I am assuming it is, but I think it may be worthwhile pointing this out in >>> the document, that unless stated otherwise, the reviewer is happy with the >>> patch >> >> I don't think there should ever be such an implication. Afaic there >> are patches I comment upon, but that I either don't feel qualified >> to give an ack/R-b on, or that I simply don't want to for whatever >> reason. At best, no other comment (as in the above example) may be >> taken as "I don't object". > > > If that is the case, would there be a reasonable convention to make this > clear? > > In a nutshell, in such a review the possible interpretations are > * I reviewed, but didn't feel qualified to do the rest > * I reviewed, but did not get round to give it full attention > * I reviewed, but before I make a final decision want to look at the next > version > ... > * I reviewed and don't object the rest > * I reviewed and agreed with the rest > The latter two are equivalent to Ack/R-b > > That is quite a large range of possibilities and kind of leaves the author > guessing what state the review is in Well, I though the convention is to give A-b / R-b explicitly. In a few overly ambiguous cases we tend to simply ask back whether a given reply can be transformed into a tag. I don't see any need for further formalization here. Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
> On 19 Dec 2019, at 09:56, Jan Beulich wrote: > > On 18.12.2019 18:09, Lars Kurth wrote: >> >> >> On 18/12/2019, 14:29, "Julien Grall" wrote: >> >>Hi Lars, >> >>On 12/12/2019 21:14, Lars Kurth wrote: >>> +### Workflow from an Author's Perspective >>> + >>> +When code authors receive feedback on their patches, they typically first >>> try >>> +to clarify feedback they do not understand. For smaller patches or patch >>> series >>> +it makes sense to wait until receiving feedback on the entire series before >>> +sending out a new version addressing the changes. For larger series, it may >>> +make sense to send out a new revision earlier. >>> + >>> +As a reviewer, you need some system that he;ps ensure that you address all >> >>Just a small typo: I think you meant "helps" rather than "he;ps". >> >>Cheers, >> >> Thank you: fixed in my working copy. >> >> One thing which occurred to me for reviews like these, where there is no >> ACK's or Reviewed-by's is that I don't actually know whether you as reviewer >> is otherwise happy with the remainder of the patch. >> Normally the ACKed-by or Reviewed-by is a signal that it is >> >> I am assuming it is, but I think it may be worthwhile pointing this out in >> the document, that unless stated otherwise, the reviewer is happy with the >> patch > > I don't think there should ever be such an implication. Afaic there > are patches I comment upon, but that I either don't feel qualified > to give an ack/R-b on, or that I simply don't want to for whatever > reason. At best, no other comment (as in the above example) may be > taken as "I don't object". If that is the case, would there be a reasonable convention to make this clear? In a nutshell, in such a review the possible interpretations are * I reviewed, but didn't feel qualified to do the rest * I reviewed, but did not get round to give it full attention * I reviewed, but before I make a final decision want to look at the next version ... * I reviewed and don't object the rest * I reviewed and agreed with the rest The latter two are equivalent to Ack/R-b That is quite a large range of possibilities and kind of leaves the author guessing what state the review is in Regards Lars ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
On 18.12.2019 18:09, Lars Kurth wrote: > > > On 18/12/2019, 14:29, "Julien Grall" wrote: > > Hi Lars, > > On 12/12/2019 21:14, Lars Kurth wrote: > > +### Workflow from an Author's Perspective > > + > > +When code authors receive feedback on their patches, they typically > first try > > +to clarify feedback they do not understand. For smaller patches or > patch series > > +it makes sense to wait until receiving feedback on the entire series > before > > +sending out a new version addressing the changes. For larger series, > it may > > +make sense to send out a new revision earlier. > > + > > +As a reviewer, you need some system that he;ps ensure that you address > all > > Just a small typo: I think you meant "helps" rather than "he;ps". > > Cheers, > > Thank you: fixed in my working copy. > > One thing which occurred to me for reviews like these, where there is no > ACK's or Reviewed-by's is that I don't actually know whether you as reviewer > is otherwise happy with the remainder of patch. > Normally the ACKed-by or Reviewed-by is a signal that it is > > I am assuming it is, but I think it may be worthwhile pointing this out in > the document, that unless stated otherwise, the reviewer is happy with the > patch I don't think there should ever be such an implication. Afaic there are patches I comment upon, but that I either don't feel qualified to give an ack/R-b on, or that I simply don't want to for whatever reason. At best, no other comment (as in the above example) may be taken as "I don't object". Jan ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
On 18/12/2019, 14:29, "Julien Grall" wrote: Hi Lars, On 12/12/2019 21:14, Lars Kurth wrote: > +### Workflow from an Author's Perspective > + > +When code authors receive feedback on their patches, they typically first try > +to clarify feedback they do not understand. For smaller patches or patch series > +it makes sense to wait until receiving feedback on the entire series before > +sending out a new version addressing the changes. For larger series, it may > +make sense to send out a new revision earlier. > + > +As a reviewer, you need some system that he;ps ensure that you address all Just a small typo: I think you meant "helps" rather than "he;ps". Cheers, Thank you: fixed in my working copy. One thing which occurred to me for reviews like these, where there is no ACK's or Reviewed-by's is that I don't actually know whether you as reviewer is otherwise happy with the remainder of patch. Normally the ACKed-by or Reviewed-by is a signal that it is I am assuming it is, but I think it may be worthwhile pointing this out in the document, that unless stated otherwise, the reviewer is happy with the patch Regards Lars ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v3 5/7] Add Code Review Guide
Hi Lars, On 12/12/2019 21:14, Lars Kurth wrote: +### Workflow from an Author's Perspective + +When code authors receive feedback on their patches, they typically first try +to clarify feedback they do not understand. For smaller patches or patch series +it makes sense to wait until receiving feedback on the entire series before +sending out a new version addressing the changes. For larger series, it may +make sense to send out a new revision earlier. + +As a reviewer, you need some system that he;ps ensure that you address all Just a small typo: I think you meant "helps" rather than "he;ps". Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH v3 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 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 TODO: find suitable examples on how to structure/describe good patch series 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 | 309 +++ 1 file changed, 309 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..5ffbbac --- /dev/null +++ b/code-review-guide.md @@ -0,0 +1,309 @@ +# 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 indentify 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 change can be easily unit tested, you should +
