[llvm-dev] [RFC] High-Level Code-Review Documentation Update

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
49 messages Options
123
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev


On Mon, Dec 2, 2019 at 8:23 PM Philip Reames <[hidden email]> wrote:


On 12/2/19 10:05 AM, David Blaikie wrote:


On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <[hidden email]> wrote:

Mehdi, David,

I think you're both pointing out exceptions rather than the general rule.  I tried to indicate their might be reasonable exceptions (see the second sentence past Mehdi's quote), but in general, particularly for new contributors, I think it is important we indicate something to this effect.  I've seen multiple new groups have issues around this.  In some cases, patches were reverted in post review.  In others, a bunch of time was sunk in a direction which turned turned out not to have wide agreement.  Cautioning folks to avoid that is important.

Do you have any suggestions on wording which keep the broad message, but make it more clear that it isn't a hard and fast rule?


My take on it is that even the broad message isn't how I think of LLVM code review practices - if the code owner/domain expert is at your company, so be it. If not, that's fine too - I see lots of reviews by people at the same company that generally look pretty good & I don't think their the exception. I'd personally leave this part out - maybe some caveat about not doing internal review & then summarily approving externally? But that I think is more the exception than the rule & perhaps not worth including in the general practices document. But if you've seen several instances of this sort of issue that seem worth addressing through this mechanism - yeah, I'd be OK with an encouragement to avoid insular project areas where possible (especially where there's strong overlap) - seek external/long-term contributor input especially on design documents and early/foundational patches, and in general err on the side of seeking more input from code owners than not. If you ask too often for trivial things, that's fine - they should hopefully get to the point where they encourage you to contribute directly/stop asking for their review/approval. But especially when asking code owners/frequent reviewers/contributors for review, be extra sure to make the patches small and clear, to have design discussion ahead of time to avoid designing in a large unwieldy code review, etc. 

Hm, I see your point, but I'm not 100% in agreement.  I'm trying to find the hair to split, but I'm struggling to find the right one.  Let's drop this for the moment from the initial patch, and I'll try to a rework on the wording in a separate review.  I don't want to risk further derailing the overall effort right now. 

Fair - certainly up for chatting about it further to try to come to some common understanding/phrasing/etc.
 
- Dave
 

Philip

On 12/2/19 7:55 AM, David Blaikie wrote:
Yeah, +1 that people from the same organization are sometimes the only ones working on a certain feature/area. (certainly I'd expect some discussion about the feature in general to be discussed outside that group if it's in any way contentious - but some stuff's clear enough (I think I implemented debug_types years ago, likely with only Eric's approval, both of us being at Google (probably many DWARF features were added/done this way, to be honest - maybe some could've done witha  bit of broader discussion, but I don't think either of us were "rubber stamping" the other's work (if anything I'm harder on my "friends" to be honest... :/ )))

On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <[hidden email]> wrote:


On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <[hidden email]> wrote:
+1 in general, and Philip has good suggestions as well!

-- 
Mehdi


On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <[hidden email]> wrote:
+ 1 in general, a couple of suggestions

On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:
> Hi, everyone,
>
> I've been fielding an increasing number of questions about how our
> code-review process in LLVM works from people who are new to our
> community, and it's been pointed out to me that our documentation on
> code reviews is both out of date and not as helpful as it could be to
> new developers.
>
>    http://llvm.org/docs/DeveloperPolicy.html#code-reviews
>
> I would like to compose a patch to update this, but before I do that, I
> want to highlight some of my thoughts to get feedback. My intent is to
> capture our community best practices in writing so that people new to
> our community understand our processes and expectations. Here are some
> things that I would like to capture:
>
>   1. You do not need to be an expert in some area of the compiler to
> review patches; it's fine to ask questions about what some piece of code
> is doing. If it's not clear to you what is going on, you're unlikely to
> be the only one. Extra comments and/or test cases can often help (and
> asking for comments in the test cases is fine as well).
Authors are encouraged to interpret questions as reasons to reexamine
the readability of the code in question.  Structural changes, or further
comments may be appropriate.
>
>   2. If you review a patch, but don't intend for the review process to
> block on your approval, please state that explicitly. Out of courtesy,
> we generally wait on committing a patch until all reviewers are
> satisfied, and if you don't intend to look at the patch again in a
> timely fashion, please communicate that fact in the review.
>
>   3. All comments by reviewers should be addressed by the patch author.
> It is generally expected that suggested changes will be incorporated
> into the next revision of the patch unless the author and/or other
> reviewers can articulate a good reason to do otherwise (and then the
> reviewers must agree). If you suggest changes in a code review, but
> don't wish the suggestion to be interpreted this strongly, please state
> so explicitly.
>
>   4. Reviewers may request certain aspects of a patch to be broken out
> into separate patches for independent review, and also, reviewers may
> accept a patch conditioned on the author providing a follow-up patch
> addressing some particular issue or concern (although no committed patch
> should leave the project in a broken state). Reviewers can also accept a
> patch conditioned on the author applying some set of minor updates prior
> to committing, and when applicable, it is polite for reviewers to do so.
>
>   5. Aim to limit the number of iterations in the review process. For
> example, when suggesting a change, if you want the author to make a
> similar set of changes at other places in the code, please explain the
> requested set of changes so that the author can make all of the changes
> at once. If a patch will require multiple steps prior to approval (e.g.,
> splitting, refactoring, posting data from specific performance tests),
> please explain as many of these up front as possible. This allows the
> patch author to make the most-efficient use of his or her time.
If the path forward is not clear - because the patch is too large to
meaningful review, or direction needs to be settled - it is fine to
suggest a clear next step (e.g. landing a refactoring) followed by a
re-review.  Please state explicitly if the path forward is unclear to
prevent confusions on the part of the author. 
>
>   6. Some changes are too large for just a code review. Changes that
> should change the Language Reference (e.g., adding new
> target-independent intrinsics), adding language extensions in Clang, and
> so on, require an RFC on *-dev first. For changes that promise
> significant impact on users and/or downstream code bases, reviewers can
> request an RFC (Request for Comment) achieving consensus before
> proceeding with code review. That having been said, posting initial
> patches can help with discussions on an RFC.
>
> Lastly, the current text reads, "Code reviews are conducted by email on
> the relevant project’s commit mailing list, or alternatively on the
> project’s development list or bug tracker.", and then only later
> mentions Phabricator. I'd like to move Phabricator to be mentioned on
> this line before the other methods.
>
> Please let me know what you think.
>
> Thanks again,
>
> Hal

A couple of additional things:

Only a single LGTM is required.  Reviewers are expected to only LGTM
patches they're confident in their knowledge of.  Reviewers may review
and provide suggestions, but explicitly defer LGTM to someone else. 
This is encouraged and a good way for new contributors to learn the code. 

There is a cultural expectation that at least one reviewer is from a
different organization than the author of the patch. 

Actually, while I'm OK with the other suggestions, I didn't pay attention to this one originally.
I'm very concerned about this: this looks like an assumption of bad faith or malice in the review process, and I find this unhealthy if it were part of the "cultural expectation". Moreover there are many areas of the compiler where there aren't many people available to review changes.

I personally never really paid attention to who is the author/reviewer of a patch from an organizational point of view, I haven't perceived this culture of looking into affiliation so far. I never got the impression that reviewer were more difficult with me than they would be with others.
There have been many patches that I reviewed that originated from other people from the same company as mine (back when I was at Apple mostly). The notion of "organization" is blurry: frequently this involved people from different teams inside the same company,  are they part of "the same organization"? Some of these people I have never even ever met or never heard of them before reviewing a patch (sometimes I don't even realize since there is a Phabricator pseudo and not everyone is using their business email here).

-- 
Mehdi



 
If that's not
possible, care should be taken to ensure overall direction has been
widely accepted. 

Post commit review is encouraged via either phabricator or email.  There
is a strong expectation that authors respond promptly to post commit
feedback and address it.  Failure to do so is cause for the patch to be
reverted.  If substantial problems are identified, it is expected that
the patch is reverted, fixed offline, and then recommitted (possibly
after further review.)


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev


On 12/3/19 9:45 AM, David Blaikie via llvm-dev wrote:


On Mon, Dec 2, 2019 at 8:23 PM Philip Reames <[hidden email]> wrote:


On 12/2/19 10:05 AM, David Blaikie wrote:


On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <[hidden email]> wrote:

Mehdi, David,

I think you're both pointing out exceptions rather than the general rule.  I tried to indicate their might be reasonable exceptions (see the second sentence past Mehdi's quote), but in general, particularly for new contributors, I think it is important we indicate something to this effect.  I've seen multiple new groups have issues around this.  In some cases, patches were reverted in post review.  In others, a bunch of time was sunk in a direction which turned turned out not to have wide agreement.  Cautioning folks to avoid that is important.

Do you have any suggestions on wording which keep the broad message, but make it more clear that it isn't a hard and fast rule?


My take on it is that even the broad message isn't how I think of LLVM code review practices - if the code owner/domain expert is at your company, so be it. If not, that's fine too - I see lots of reviews by people at the same company that generally look pretty good & I don't think their the exception. I'd personally leave this part out - maybe some caveat about not doing internal review & then summarily approving externally? But that I think is more the exception than the rule & perhaps not worth including in the general practices document. But if you've seen several instances of this sort of issue that seem worth addressing through this mechanism - yeah, I'd be OK with an encouragement to avoid insular project areas where possible (especially where there's strong overlap) - seek external/long-term contributor input especially on design documents and early/foundational patches, and in general err on the side of seeking more input from code owners than not. If you ask too often for trivial things, that's fine - they should hopefully get to the point where they encourage you to contribute directly/stop asking for their review/approval. But especially when asking code owners/frequent reviewers/contributors for review, be extra sure to make the patches small and clear, to have design discussion ahead of time to avoid designing in a large unwieldy code review, etc. 

Hm, I see your point, but I'm not 100% in agreement.  I'm trying to find the hair to split, but I'm struggling to find the right one.  Let's drop this for the moment from the initial patch, and I'll try to a rework on the wording in a separate review.  I don't want to risk further derailing the overall effort right now. 

Fair - certainly up for chatting about it further to try to come to some common understanding/phrasing/etc.


I feel like the underlying point is that we want to ensure community consensus around design decisions, and one responsibility of a reviewer, when providing an overall approval for a patch, is to be reasonable sure that such consensus exists. Maybe that means making sure that people from multiple organizations likely to care about a particular issue have had a chance to review, and/or have been specifically pinged, and maybe not. One way of thinking about it may be that if you're not familiar enough with the community to know, then you shouldn't be providing final approval to commit.

My preference is to start with some high-level guideline at this level, and then we can develop more-specific guidelines as needed. I don't really want to develop specific organization-based rules, unless we really need to, in part because the decomposition of legal entities is often arbitrary in this context.

 -Hal


 
- Dave
 

Philip

On 12/2/19 7:55 AM, David Blaikie wrote:
Yeah, +1 that people from the same organization are sometimes the only ones working on a certain feature/area. (certainly I'd expect some discussion about the feature in general to be discussed outside that group if it's in any way contentious - but some stuff's clear enough (I think I implemented debug_types years ago, likely with only Eric's approval, both of us being at Google (probably many DWARF features were added/done this way, to be honest - maybe some could've done witha  bit of broader discussion, but I don't think either of us were "rubber stamping" the other's work (if anything I'm harder on my "friends" to be honest... :/ )))

On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <[hidden email]> wrote:


On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <[hidden email]> wrote:
+1 in general, and Philip has good suggestions as well!

-- 
Mehdi


On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <[hidden email]> wrote:
+ 1 in general, a couple of suggestions

On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:
> Hi, everyone,
>
> I've been fielding an increasing number of questions about how our
> code-review process in LLVM works from people who are new to our
> community, and it's been pointed out to me that our documentation on
> code reviews is both out of date and not as helpful as it could be to
> new developers.
>
>    http://llvm.org/docs/DeveloperPolicy.html#code-reviews
>
> I would like to compose a patch to update this, but before I do that, I
> want to highlight some of my thoughts to get feedback. My intent is to
> capture our community best practices in writing so that people new to
> our community understand our processes and expectations. Here are some
> things that I would like to capture:
>
>   1. You do not need to be an expert in some area of the compiler to
> review patches; it's fine to ask questions about what some piece of code
> is doing. If it's not clear to you what is going on, you're unlikely to
> be the only one. Extra comments and/or test cases can often help (and
> asking for comments in the test cases is fine as well).
Authors are encouraged to interpret questions as reasons to reexamine
the readability of the code in question.  Structural changes, or further
comments may be appropriate.
>
>   2. If you review a patch, but don't intend for the review process to
> block on your approval, please state that explicitly. Out of courtesy,
> we generally wait on committing a patch until all reviewers are
> satisfied, and if you don't intend to look at the patch again in a
> timely fashion, please communicate that fact in the review.
>
>   3. All comments by reviewers should be addressed by the patch author.
> It is generally expected that suggested changes will be incorporated
> into the next revision of the patch unless the author and/or other
> reviewers can articulate a good reason to do otherwise (and then the
> reviewers must agree). If you suggest changes in a code review, but
> don't wish the suggestion to be interpreted this strongly, please state
> so explicitly.
>
>   4. Reviewers may request certain aspects of a patch to be broken out
> into separate patches for independent review, and also, reviewers may
> accept a patch conditioned on the author providing a follow-up patch
> addressing some particular issue or concern (although no committed patch
> should leave the project in a broken state). Reviewers can also accept a
> patch conditioned on the author applying some set of minor updates prior
> to committing, and when applicable, it is polite for reviewers to do so.
>
>   5. Aim to limit the number of iterations in the review process. For
> example, when suggesting a change, if you want the author to make a
> similar set of changes at other places in the code, please explain the
> requested set of changes so that the author can make all of the changes
> at once. If a patch will require multiple steps prior to approval (e.g.,
> splitting, refactoring, posting data from specific performance tests),
> please explain as many of these up front as possible. This allows the
> patch author to make the most-efficient use of his or her time.
If the path forward is not clear - because the patch is too large to
meaningful review, or direction needs to be settled - it is fine to
suggest a clear next step (e.g. landing a refactoring) followed by a
re-review.  Please state explicitly if the path forward is unclear to
prevent confusions on the part of the author. 
>
>   6. Some changes are too large for just a code review. Changes that
> should change the Language Reference (e.g., adding new
> target-independent intrinsics), adding language extensions in Clang, and
> so on, require an RFC on *-dev first. For changes that promise
> significant impact on users and/or downstream code bases, reviewers can
> request an RFC (Request for Comment) achieving consensus before
> proceeding with code review. That having been said, posting initial
> patches can help with discussions on an RFC.
>
> Lastly, the current text reads, "Code reviews are conducted by email on
> the relevant project’s commit mailing list, or alternatively on the
> project’s development list or bug tracker.", and then only later
> mentions Phabricator. I'd like to move Phabricator to be mentioned on
> this line before the other methods.
>
> Please let me know what you think.
>
> Thanks again,
>
> Hal

A couple of additional things:

Only a single LGTM is required.  Reviewers are expected to only LGTM
patches they're confident in their knowledge of.  Reviewers may review
and provide suggestions, but explicitly defer LGTM to someone else. 
This is encouraged and a good way for new contributors to learn the code. 

There is a cultural expectation that at least one reviewer is from a
different organization than the author of the patch. 

Actually, while I'm OK with the other suggestions, I didn't pay attention to this one originally.
I'm very concerned about this: this looks like an assumption of bad faith or malice in the review process, and I find this unhealthy if it were part of the "cultural expectation". Moreover there are many areas of the compiler where there aren't many people available to review changes.

I personally never really paid attention to who is the author/reviewer of a patch from an organizational point of view, I haven't perceived this culture of looking into affiliation so far. I never got the impression that reviewer were more difficult with me than they would be with others.
There have been many patches that I reviewed that originated from other people from the same company as mine (back when I was at Apple mostly). The notion of "organization" is blurry: frequently this involved people from different teams inside the same company,  are they part of "the same organization"? Some of these people I have never even ever met or never heard of them before reviewing a patch (sometimes I don't even realize since there is a Phabricator pseudo and not everyone is using their business email here).

-- 
Mehdi



 
If that's not
possible, care should be taken to ensure overall direction has been
widely accepted. 

Post commit review is encouraged via either phabricator or email.  There
is a strong expectation that authors respond promptly to post commit
feedback and address it.  Failure to do so is cause for the patch to be
reverted.  If substantial problems are identified, it is expected that
the patch is reverted, fixed offline, and then recommitted (possibly
after further review.)


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev
In reply to this post by David Blaikie via llvm-dev


On 12/2/19 10:52 AM, Hubert Tong wrote:
On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev <[hidden email]> wrote:
  3. All comments by reviewers should be addressed by the patch author.
It is generally expected that suggested changes will be incorporated
into the next revision of the patch unless the author and/or other
reviewers can articulate a good reason to do otherwise (and then the
reviewers must agree).
I disagree on the high bar here. The author should acknowledge the comments; however, addressing all of the comments in one shot has similar problems as having commits that are too large (diffs between revisions become more difficult to review).


I'm happy to use acknowledge instead of address. That's probably closer to what I intended. It is certainly true that separable enhancements (i.e., not required for correctness) are normally better as follow up patches, and we should make sure that's clear.


This also leads to significant timing issues, where the comments made overnight in some time zone are addressed by the author locally, but someone added comments in the afternoon the next day before the author has a chance to post the new revision.


Good point. We should say "in a future revision"; there are a number of reasons why updates might be staged.

 -Hal


 
If you suggest changes in a code review, but
don't wish the suggestion to be interpreted this strongly, please state
so explicitly.
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev
In reply to this post by David Blaikie via llvm-dev


On 12/3/19 8:26 AM, Finkel, Hal J. wrote:


On 12/3/19 9:45 AM, David Blaikie via llvm-dev wrote:


On Mon, Dec 2, 2019 at 8:23 PM Philip Reames <[hidden email]> wrote:


On 12/2/19 10:05 AM, David Blaikie wrote:


On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <[hidden email]> wrote:

Mehdi, David,

I think you're both pointing out exceptions rather than the general rule.  I tried to indicate their might be reasonable exceptions (see the second sentence past Mehdi's quote), but in general, particularly for new contributors, I think it is important we indicate something to this effect.  I've seen multiple new groups have issues around this.  In some cases, patches were reverted in post review.  In others, a bunch of time was sunk in a direction which turned turned out not to have wide agreement.  Cautioning folks to avoid that is important.

Do you have any suggestions on wording which keep the broad message, but make it more clear that it isn't a hard and fast rule?


My take on it is that even the broad message isn't how I think of LLVM code review practices - if the code owner/domain expert is at your company, so be it. If not, that's fine too - I see lots of reviews by people at the same company that generally look pretty good & I don't think their the exception. I'd personally leave this part out - maybe some caveat about not doing internal review & then summarily approving externally? But that I think is more the exception than the rule & perhaps not worth including in the general practices document. But if you've seen several instances of this sort of issue that seem worth addressing through this mechanism - yeah, I'd be OK with an encouragement to avoid insular project areas where possible (especially where there's strong overlap) - seek external/long-term contributor input especially on design documents and early/foundational patches, and in general err on the side of seeking more input from code owners than not. If you ask too often for trivial things, that's fine - they should hopefully get to the point where they encourage you to contribute directly/stop asking for their review/approval. But especially when asking code owners/frequent reviewers/contributors for review, be extra sure to make the patches small and clear, to have design discussion ahead of time to avoid designing in a large unwieldy code review, etc. 

Hm, I see your point, but I'm not 100% in agreement.  I'm trying to find the hair to split, but I'm struggling to find the right one.  Let's drop this for the moment from the initial patch, and I'll try to a rework on the wording in a separate review.  I don't want to risk further derailing the overall effort right now. 

Fair - certainly up for chatting about it further to try to come to some common understanding/phrasing/etc.


I feel like the underlying point is that we want to ensure community consensus around design decisions, and one responsibility of a reviewer, when providing an overall approval for a patch, is to be reasonable sure that such consensus exists. Maybe that means making sure that people from multiple organizations likely to care about a particular issue have had a chance to review, and/or have been specifically pinged, and maybe not. One way of thinking about it may be that if you're not familiar enough with the community to know, then you shouldn't be providing final approval to commit.

I like this framing.  It gets at the core point while avoiding the concerns raised.

My preference is to start with some high-level guideline at this level, and then we can develop more-specific guidelines as needed. I don't really want to develop specific organization-based rules, unless we really need to, in part because the decomposition of legal entities is often arbitrary in this context.

 -Hal


 
- Dave
 

Philip

On 12/2/19 7:55 AM, David Blaikie wrote:
Yeah, +1 that people from the same organization are sometimes the only ones working on a certain feature/area. (certainly I'd expect some discussion about the feature in general to be discussed outside that group if it's in any way contentious - but some stuff's clear enough (I think I implemented debug_types years ago, likely with only Eric's approval, both of us being at Google (probably many DWARF features were added/done this way, to be honest - maybe some could've done witha  bit of broader discussion, but I don't think either of us were "rubber stamping" the other's work (if anything I'm harder on my "friends" to be honest... :/ )))

On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev <[hidden email]> wrote:


On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <[hidden email]> wrote:
+1 in general, and Philip has good suggestions as well!

-- 
Mehdi


On Sat, Nov 16, 2019 at 8:37 AM Philip Reames via llvm-dev <[hidden email]> wrote:
+ 1 in general, a couple of suggestions

On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev wrote:
> Hi, everyone,
>
> I've been fielding an increasing number of questions about how our
> code-review process in LLVM works from people who are new to our
> community, and it's been pointed out to me that our documentation on
> code reviews is both out of date and not as helpful as it could be to
> new developers.
>
>    http://llvm.org/docs/DeveloperPolicy.html#code-reviews
>
> I would like to compose a patch to update this, but before I do that, I
> want to highlight some of my thoughts to get feedback. My intent is to
> capture our community best practices in writing so that people new to
> our community understand our processes and expectations. Here are some
> things that I would like to capture:
>
>   1. You do not need to be an expert in some area of the compiler to
> review patches; it's fine to ask questions about what some piece of code
> is doing. If it's not clear to you what is going on, you're unlikely to
> be the only one. Extra comments and/or test cases can often help (and
> asking for comments in the test cases is fine as well).
Authors are encouraged to interpret questions as reasons to reexamine
the readability of the code in question.  Structural changes, or further
comments may be appropriate.
>
>   2. If you review a patch, but don't intend for the review process to
> block on your approval, please state that explicitly. Out of courtesy,
> we generally wait on committing a patch until all reviewers are
> satisfied, and if you don't intend to look at the patch again in a
> timely fashion, please communicate that fact in the review.
>
>   3. All comments by reviewers should be addressed by the patch author.
> It is generally expected that suggested changes will be incorporated
> into the next revision of the patch unless the author and/or other
> reviewers can articulate a good reason to do otherwise (and then the
> reviewers must agree). If you suggest changes in a code review, but
> don't wish the suggestion to be interpreted this strongly, please state
> so explicitly.
>
>   4. Reviewers may request certain aspects of a patch to be broken out
> into separate patches for independent review, and also, reviewers may
> accept a patch conditioned on the author providing a follow-up patch
> addressing some particular issue or concern (although no committed patch
> should leave the project in a broken state). Reviewers can also accept a
> patch conditioned on the author applying some set of minor updates prior
> to committing, and when applicable, it is polite for reviewers to do so.
>
>   5. Aim to limit the number of iterations in the review process. For
> example, when suggesting a change, if you want the author to make a
> similar set of changes at other places in the code, please explain the
> requested set of changes so that the author can make all of the changes
> at once. If a patch will require multiple steps prior to approval (e.g.,
> splitting, refactoring, posting data from specific performance tests),
> please explain as many of these up front as possible. This allows the
> patch author to make the most-efficient use of his or her time.
If the path forward is not clear - because the patch is too large to
meaningful review, or direction needs to be settled - it is fine to
suggest a clear next step (e.g. landing a refactoring) followed by a
re-review.  Please state explicitly if the path forward is unclear to
prevent confusions on the part of the author. 
>
>   6. Some changes are too large for just a code review. Changes that
> should change the Language Reference (e.g., adding new
> target-independent intrinsics), adding language extensions in Clang, and
> so on, require an RFC on *-dev first. For changes that promise
> significant impact on users and/or downstream code bases, reviewers can
> request an RFC (Request for Comment) achieving consensus before
> proceeding with code review. That having been said, posting initial
> patches can help with discussions on an RFC.
>
> Lastly, the current text reads, "Code reviews are conducted by email on
> the relevant project’s commit mailing list, or alternatively on the
> project’s development list or bug tracker.", and then only later
> mentions Phabricator. I'd like to move Phabricator to be mentioned on
> this line before the other methods.
>
> Please let me know what you think.
>
> Thanks again,
>
> Hal

A couple of additional things:

Only a single LGTM is required.  Reviewers are expected to only LGTM
patches they're confident in their knowledge of.  Reviewers may review
and provide suggestions, but explicitly defer LGTM to someone else. 
This is encouraged and a good way for new contributors to learn the code. 

There is a cultural expectation that at least one reviewer is from a
different organization than the author of the patch. 

Actually, while I'm OK with the other suggestions, I didn't pay attention to this one originally.
I'm very concerned about this: this looks like an assumption of bad faith or malice in the review process, and I find this unhealthy if it were part of the "cultural expectation". Moreover there are many areas of the compiler where there aren't many people available to review changes.

I personally never really paid attention to who is the author/reviewer of a patch from an organizational point of view, I haven't perceived this culture of looking into affiliation so far. I never got the impression that reviewer were more difficult with me than they would be with others.
There have been many patches that I reviewed that originated from other people from the same company as mine (back when I was at Apple mostly). The notion of "organization" is blurry: frequently this involved people from different teams inside the same company,  are they part of "the same organization"? Some of these people I have never even ever met or never heard of them before reviewing a patch (sometimes I don't even realize since there is a Phabricator pseudo and not everyone is using their business email here).

-- 
Mehdi



 
If that's not
possible, care should be taken to ensure overall direction has been
widely accepted. 

Post commit review is encouraged via either phabricator or email.  There
is a strong expectation that authors respond promptly to post commit
feedback and address it.  Failure to do so is cause for the patch to be
reverted.  If substantial problems are identified, it is expected that
the patch is reverted, fixed offline, and then recommitted (possibly
after further review.)


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev
In reply to this post by David Blaikie via llvm-dev


On Mon, Dec 2, 2019 at 9:48 AM Philip Reames via llvm-dev <[hidden email]> wrote:


On 12/2/19 8:52 AM, Hubert Tong via llvm-dev wrote:
On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev <[hidden email]> wrote:
  3. All comments by reviewers should be addressed by the patch author.
It is generally expected that suggested changes will be incorporated
into the next revision of the patch unless the author and/or other
reviewers can articulate a good reason to do otherwise (and then the
reviewers must agree).
I disagree on the high bar here. The author should acknowledge the comments; however, addressing all of the comments in one shot has similar problems as having commits that are too large (diffs between revisions become more difficult to review). This also leads to significant timing issues, where the comments made overnight in some time zone are addressed by the author locally, but someone added comments in the afternoon the next day before the author has a chance to post the new revision.

Then the author needs to indicate this explicitly, and except in exceptional circumstances with an explicit request, should not expect re-review until comments are addressed.  It's fine to post a new version of the patch; just not to expect action by reviewers. 

I see this one as a major stumbling block for new contributors - i.e. reviews get stuck because both sides expect the other to be doing something.  Having it clearly documented is important IMO. 



Does phabricator has some sort of functionality to formalize this? I feel like I’ve seen that somewhere on phab (it wasn’t the llvm phab instance though).

One of the code review systems I’m using these days has a feature that a patch stays “pending” until not only LGTM is given, but also all comments have been marked as acknowledged/resolved in the UI, at which point the web UI turns green and is “ready” to submit.

— Sean Silva


 
If you suggest changes in a code review, but
don't wish the suggestion to be interpreted this strongly, please state
so explicitly.

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev

I have seen a reviewer mark the review as “changes required” and an author mark it as “changes planned.”

I haven’t seen these used very often, and only for something pretty significant.  As it’s not common practice I’d be reluctant to try to codify it in a policy.

--paulr

 

From: llvm-dev <[hidden email]> On Behalf Of Sean Silva via llvm-dev
Sent: Wednesday, December 4, 2019 9:12 PM
To: Philip Reames <[hidden email]>
Cc: [hidden email]
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

 

 

 

On Mon, Dec 2, 2019 at 9:48 AM Philip Reames via llvm-dev <[hidden email]> wrote:

 

On 12/2/19 8:52 AM, Hubert Tong via llvm-dev wrote:

On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev <[hidden email]> wrote:

  3. All comments by reviewers should be addressed by the patch author.
It is generally expected that suggested changes will be incorporated
into the next revision of the patch unless the author and/or other
reviewers can articulate a good reason to do otherwise (and then the
reviewers must agree).

I disagree on the high bar here. The author should acknowledge the comments; however, addressing all of the comments in one shot has similar problems as having commits that are too large (diffs between revisions become more difficult to review). This also leads to significant timing issues, where the comments made overnight in some time zone are addressed by the author locally, but someone added comments in the afternoon the next day before the author has a chance to post the new revision.

Then the author needs to indicate this explicitly, and except in exceptional circumstances with an explicit request, should not expect re-review until comments are addressed.  It's fine to post a new version of the patch; just not to expect action by reviewers. 

I see this one as a major stumbling block for new contributors - i.e. reviews get stuck because both sides expect the other to be doing something.  Having it clearly documented is important IMO. 

 

 

Does phabricator has some sort of functionality to formalize this? I feel like I’ve seen that somewhere on phab (it wasn’t the llvm phab instance though).

 

One of the code review systems I’m using these days has a feature that a patch stays “pending” until not only LGTM is given, but also all comments have been marked as acknowledged/resolved in the UI, at which point the web UI turns green and is “ready” to submit.

 

— Sean Silva

 

 

If you suggest changes in a code review, but
don't wish the suggestion to be interpreted this strongly, please state
so explicitly.

 

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev


On 12/5/19 8:50 AM, Robinson, Paul via llvm-dev wrote:

I have seen a reviewer mark the review as “changes required” and an author mark it as “changes planned.”

I haven’t seen these used very often, and only for something pretty significant.  As it’s not common practice I’d be reluctant to try to codify it in a policy.


The best practice I've seen, and one that I'll suggest we document, is that if a new patch does not address all outstanding feedback, the author should explicitly state that in the patch description.

  -Hal


--paulr

 

From: llvm-dev [hidden email] On Behalf Of Sean Silva via llvm-dev
Sent: Wednesday, December 4, 2019 9:12 PM
To: Philip Reames [hidden email]
Cc: [hidden email]
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

 

 

 

On Mon, Dec 2, 2019 at 9:48 AM Philip Reames via llvm-dev <[hidden email]> wrote:

 

On 12/2/19 8:52 AM, Hubert Tong via llvm-dev wrote:

On Thu, Nov 14, 2019 at 10:46 PM Finkel, Hal J. via llvm-dev <[hidden email]> wrote:

  3. All comments by reviewers should be addressed by the patch author.
It is generally expected that suggested changes will be incorporated
into the next revision of the patch unless the author and/or other
reviewers can articulate a good reason to do otherwise (and then the
reviewers must agree).

I disagree on the high bar here. The author should acknowledge the comments; however, addressing all of the comments in one shot has similar problems as having commits that are too large (diffs between revisions become more difficult to review). This also leads to significant timing issues, where the comments made overnight in some time zone are addressed by the author locally, but someone added comments in the afternoon the next day before the author has a chance to post the new revision.

Then the author needs to indicate this explicitly, and except in exceptional circumstances with an explicit request, should not expect re-review until comments are addressed.  It's fine to post a new version of the patch; just not to expect action by reviewers. 

I see this one as a major stumbling block for new contributors - i.e. reviews get stuck because both sides expect the other to be doing something.  Having it clearly documented is important IMO. 

 

 

Does phabricator has some sort of functionality to formalize this? I feel like I’ve seen that somewhere on phab (it wasn’t the llvm phab instance though).

 

One of the code review systems I’m using these days has a feature that a patch stays “pending” until not only LGTM is given, but also all comments have been marked as acknowledged/resolved in the UI, at which point the web UI turns green and is “ready” to submit.

 

— Sean Silva

 

 

If you suggest changes in a code review, but
don't wish the suggestion to be interpreted this strongly, please state
so explicitly.

 

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev
On Thu, 5 Dec 2019 at 15:35, Finkel, Hal J. via llvm-dev
<[hidden email]> wrote:
> The best practice I've seen, and one that I'll suggest we document, is that if a new patch does not address all outstanding feedback, the author should explicitly state that in the patch description.

+1!

For example: to be addressed by a following patch, or assumption no
longer valid, etc.
_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

David Blaikie via llvm-dev
In reply to this post by David Blaikie via llvm-dev
I would like to thank everyone who provided feedback on this thread. I
attempted to incorporate all of it. If I missed something, please let me
know.

I've posted a patch to our documentation here:
https://reviews.llvm.org/D71916

  -Hal

On 11/14/19 9:45 PM, Hal Finkel wrote:

> Hi, everyone,
>
> I've been fielding an increasing number of questions about how our
> code-review process in LLVM works from people who are new to our
> community, and it's been pointed out to me that our documentation on
> code reviews is both out of date and not as helpful as it could be to
> new developers.
>
>   http://llvm.org/docs/DeveloperPolicy.html#code-reviews
>
> I would like to compose a patch to update this, but before I do that,
> I want to highlight some of my thoughts to get feedback. My intent is
> to capture our community best practices in writing so that people new
> to our community understand our processes and expectations. Here are
> some things that I would like to capture:
>
>  1. You do not need to be an expert in some area of the compiler to
> review patches; it's fine to ask questions about what some piece of
> code is doing. If it's not clear to you what is going on, you're
> unlikely to be the only one. Extra comments and/or test cases can
> often help (and asking for comments in the test cases is fine as well).
>
>  2. If you review a patch, but don't intend for the review process to
> block on your approval, please state that explicitly. Out of courtesy,
> we generally wait on committing a patch until all reviewers are
> satisfied, and if you don't intend to look at the patch again in a
> timely fashion, please communicate that fact in the review.
>
>  3. All comments by reviewers should be addressed by the patch author.
> It is generally expected that suggested changes will be incorporated
> into the next revision of the patch unless the author and/or other
> reviewers can articulate a good reason to do otherwise (and then the
> reviewers must agree). If you suggest changes in a code review, but
> don't wish the suggestion to be interpreted this strongly, please
> state so explicitly.
>
>  4. Reviewers may request certain aspects of a patch to be broken out
> into separate patches for independent review, and also, reviewers may
> accept a patch conditioned on the author providing a follow-up patch
> addressing some particular issue or concern (although no committed
> patch should leave the project in a broken state). Reviewers can also
> accept a patch conditioned on the author applying some set of minor
> updates prior to committing, and when applicable, it is polite for
> reviewers to do so.
>
>  5. Aim to limit the number of iterations in the review process. For
> example, when suggesting a change, if you want the author to make a
> similar set of changes at other places in the code, please explain the
> requested set of changes so that the author can make all of the
> changes at once. If a patch will require multiple steps prior to
> approval (e.g., splitting, refactoring, posting data from specific
> performance tests), please explain as many of these up front as
> possible. This allows the patch author to make the most-efficient use
> of his or her time.
>
>  6. Some changes are too large for just a code review. Changes that
> should change the Language Reference (e.g., adding new
> target-independent intrinsics), adding language extensions in Clang,
> and so on, require an RFC on *-dev first. For changes that promise
> significant impact on users and/or downstream code bases, reviewers
> can request an RFC (Request for Comment) achieving consensus before
> proceeding with code review. That having been said, posting initial
> patches can help with discussions on an RFC.
>
> Lastly, the current text reads, "Code reviews are conducted by email
> on the relevant project’s commit mailing list, or alternatively on the
> project’s development list or bug tracker.", and then only later
> mentions Phabricator. I'd like to move Phabricator to be mentioned on
> this line before the other methods.
>
> Please let me know what you think.
>
> Thanks again,
>
> Hal
>
--
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

_______________________________________________
LLVM Developers mailing list
[hidden email]
https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
123