[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

Jeremy Morse via llvm-dev
On Mon, 18 Nov 2019 at 15:53, Finkel, Hal J. via llvm-dev
<[hidden email]> wrote:
> I agree with this. I was planning on proposing wording along the lines of the following, adding to the original suggestion:
>
> When providing an unqualified LGTM (approval to commit), it is the responsibility of the reviewer to have reviewed all of the discussion and feedback from all reviewers ensuring that all feedback has been addressed and that all other reviewers will almost surely be satisfied with the patch being approved. If unsure, the reviewer should provide a qualified approval, (e.g., "LGTM, but please wait for @someone, @someone_else"). You may also do this if you are fairly certain that a particular community member will wish to review, even if that person hasn't done so yet (although don't wait for more than one week if that person has not responded; if you think something is "must see" by a wider audience, it should have an RFC). If it is likely that others will want to review a recently-posted patch, especially if there might be objections, but no one else has done so yet, it is also polite to provide a qualified approval (e.g., "LGTM, but please wait for a couple days in case others wish to review").
>
> Thoughts?

I felt your original proposal was missing this part, but everything
else was fine. So adding this, LGTM. :)

Maybe this paragraph is a bit convoluted and too big, but that's not
important, as long as we pass the information.

One comment on the format, though...

Not everyone reads all paragraphs, but all of them are important. I
have two tactics that make sure people pay attention to what's
necessary:

1. The first sentence of the bullet point is very short, and _conveys
the whole point_.

The paragraph can continue, expanding on the meaning, but the number
of people that will read is reduced, like 1/x for each new word. If
people understand and agree with the sentence, not much else is
needed. If they disagree or don't understand, they can go as far as
they need to make sense of it. An example is my phrase up there, and
this paragraph here. :)

2. Make the first sentence in **bold**.

This helps quick-reading and finding information, especially on long
pages/sections. Further italics or underline of specific core words
can be done, if necessary. Honestly, I could have said "short and
bold" on the first bullet point, but I wanted to have two, to
demonstrate the point, as the first sentece was already too long and
people won't read the word "bold", even if it was in bold.

cheers,
--renato
_______________________________________________
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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
Perhaps rather than a one-week auto-cutoff, it might be worth saying something like "(if after a week that person hasn't responded, feel free to ping them, or ask the original reviewer for further suggestions...)". It can probably be worded better than that though. An issue with just one week is that if someone is off work, a week is a common period to be off for and therefore they might need 10 days to get back. Not sure if there's a clear solution to this, and I think it might depend on the situation really.

James

On Mon, 18 Nov 2019 at 16:58, David Blaikie <[hidden email]> wrote:


On Mon, Nov 18, 2019 at 7:53 AM Finkel, Hal J. via llvm-dev <[hidden email]> wrote:


On 11/18/19 4:29 AM, James Henderson wrote:
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.
Whilst I get what you're trying to say, I'm not particularly comfortable with this particular suggestion as it stands: I regularly am one of two or three active reviewers on something, often spread out over different timezones, and I might be happy with it, in which case I'd signal that with an LGTM, but others might not be ready for it to be committed. Sometimes I'll ask for the author to wait to commit until one or more of the other reviewers are happy too, but other times I forget to explicitly say this. Perhaps a couple of sentences could be added to the end of this paragraph to capture this approach:

"If multiple reviewers have been actively reviewing the patch, it is generally courteous to allow all of them a chance to give their LGTM before committing, after one LGTM has been received. The reviewer who gives the original LGTM may suggest an appropriate amount of time to wait before committing in this case."

What I want to avoid is me (UK timezone) making some suggestions on a patch proposed by someone e.g. in the US, then a reviewer from the US getting into an active discussion, proposing a counter-suggestion, which gets adopted and LGTMed by that reviewer, resulting in a commit before I've had a chance to follow up on my comments etc. Obviously I can make post-commit requests, but sometimes it feels like the bar for suggestions post-commit is higher, and therefore my comments might not reach that level etc.


I agree with this. I was planning on proposing wording along the lines of the following, adding to the original suggestion:

When providing an unqualified LGTM (approval to commit), it is the responsibility of the reviewer to have reviewed all of the discussion and feedback from all reviewers ensuring that all feedback has been addressed and that all other reviewers will almost surely be satisfied with the patch being approved. If unsure, the reviewer should provide a qualified approval, (e.g., "LGTM, but please wait for @someone, @someone_else"). You may also do this if you are fairly certain that a particular community member will wish to review, even if that person hasn't done so yet (although don't wait for more than one week if that person has not responded; if you think something is "must see" by a wider audience, it should have an RFC). If it is likely that others will want to review a recently-posted patch, especially if there might be objections, but no one else has done so yet, it is also polite to provide a qualified approval (e.g., "LGTM, but please wait for a couple days in case others wish to review"). 

Thoughts?

Generally sounds good to me though " (although don't wait for more than one week if that person has not responded; if you think something is "must see" by a wider audience, it should have an RFC)" - makes me a bit twitchy. I think it's pretty reasonable for someone to say "LGTM, but also wait for @someone" without having a week deadline. I do that pretty often - hoping to provide a first pass review but there being a design decision or the like that I feel is out of my depth and needs sign off from a code owner or similar, essentially.
 

 -Hal



James

On Sat, 16 Nov 2019 at 16:37, 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.  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
-- 
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

_______________________________________________
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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
On Mon, Nov 18, 2019 at 4:53 PM Finkel, Hal J. via llvm-dev
<[hidden email]> wrote:

> On 11/18/19 4:29 AM, James Henderson wrote:
>>
>> 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.
>
> Whilst I get what you're trying to say, I'm not particularly comfortable with this particular suggestion as it stands: I regularly am one of two or three active reviewers on something, often spread out over different timezones, and I might be happy with it, in which case I'd signal that with an LGTM, but others might not be ready for it to be committed. Sometimes I'll ask for the author to wait to commit until one or more of the other reviewers are happy too, but other times I forget to explicitly say this. Perhaps a couple of sentences could be added to the end of this paragraph to capture this approach:
>
> "If multiple reviewers have been actively reviewing the patch, it is generally courteous to allow all of them a chance to give their LGTM before committing, after one LGTM has been received. The reviewer who gives the original LGTM may suggest an appropriate amount of time to wait before committing in this case."
>
> What I want to avoid is me (UK timezone) making some suggestions on a patch proposed by someone e.g. in the US, then a reviewer from the US getting into an active discussion, proposing a counter-suggestion, which gets adopted and LGTMed by that reviewer, resulting in a commit before I've had a chance to follow up on my comments etc. Obviously I can make post-commit requests, but sometimes it feels like the bar for suggestions post-commit is higher, and therefore my comments might not reach that level etc.
>
>
> I agree with this. I was planning on proposing wording along the lines of the following, adding to the original suggestion:
>
> When providing an unqualified LGTM (approval to commit), it is the responsibility of the reviewer to have reviewed all of the discussion and feedback from all reviewers ensuring that all feedback has been addressed and that all other reviewers will almost surely be satisfied with the patch being approved. If unsure, the reviewer should provide a qualified approval, (e.g., "LGTM, but please wait for @someone, @someone_else"). You may also do this if you are fairly certain that a particular community member will wish to review, even if that person hasn't done so yet (although don't wait for more than one week if that person has not responded; if you think something is "must see" by a wider audience, it should have an RFC). If it is likely that others will want to review a recently-posted patch, especially if there might be objections, but no one else has done so yet, it is also polite to provide a qualified approval (e.g., "LGTM, but please wait for a couple days in case others wish to review").

On a somewhat related note: Due to timezones, I think good etiquette
is to wait a reasonable time before committing a patch if you receive
a LGTM very quickly. I think non-trivial patches should generally be
up for at least 24 hours -- maybe even extend that to 48 hours --
before committing.

Of course, this is usually not an issue in LLVM today because we're so
bad at reviewing code in general :) If you've waited on a review for a
month and pinged people several times, then by all means feel free to
commit immediately after getting a LGTM.

Cheers,
Nicolai



>
> Thoughts?
>
>  -Hal
>
>
>
> James
>
> On Sat, 16 Nov 2019 at 16:37, 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.  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
>
> --
> 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



--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
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

Jeremy Morse via llvm-dev


On Wed, Nov 20, 2019 at 2:28 AM Nicolai Hähnle via llvm-dev <[hidden email]> wrote:
On Mon, Nov 18, 2019 at 4:53 PM Finkel, Hal J. via llvm-dev
<[hidden email]> wrote:
> On 11/18/19 4:29 AM, James Henderson wrote:
>>
>> 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.
>
> Whilst I get what you're trying to say, I'm not particularly comfortable with this particular suggestion as it stands: I regularly am one of two or three active reviewers on something, often spread out over different timezones, and I might be happy with it, in which case I'd signal that with an LGTM, but others might not be ready for it to be committed. Sometimes I'll ask for the author to wait to commit until one or more of the other reviewers are happy too, but other times I forget to explicitly say this. Perhaps a couple of sentences could be added to the end of this paragraph to capture this approach:
>
> "If multiple reviewers have been actively reviewing the patch, it is generally courteous to allow all of them a chance to give their LGTM before committing, after one LGTM has been received. The reviewer who gives the original LGTM may suggest an appropriate amount of time to wait before committing in this case."
>
> What I want to avoid is me (UK timezone) making some suggestions on a patch proposed by someone e.g. in the US, then a reviewer from the US getting into an active discussion, proposing a counter-suggestion, which gets adopted and LGTMed by that reviewer, resulting in a commit before I've had a chance to follow up on my comments etc. Obviously I can make post-commit requests, but sometimes it feels like the bar for suggestions post-commit is higher, and therefore my comments might not reach that level etc.
>
>
> I agree with this. I was planning on proposing wording along the lines of the following, adding to the original suggestion:
>
> When providing an unqualified LGTM (approval to commit), it is the responsibility of the reviewer to have reviewed all of the discussion and feedback from all reviewers ensuring that all feedback has been addressed and that all other reviewers will almost surely be satisfied with the patch being approved. If unsure, the reviewer should provide a qualified approval, (e.g., "LGTM, but please wait for @someone, @someone_else"). You may also do this if you are fairly certain that a particular community member will wish to review, even if that person hasn't done so yet (although don't wait for more than one week if that person has not responded; if you think something is "must see" by a wider audience, it should have an RFC). If it is likely that others will want to review a recently-posted patch, especially if there might be objections, but no one else has done so yet, it is also polite to provide a qualified approval (e.g., "LGTM, but please wait for a couple days in case others wish to review").

On a somewhat related note: Due to timezones, I think good etiquette
is to wait a reasonable time before committing a patch if you receive
a LGTM very quickly. I think non-trivial patches should generally be
up for at least 24 hours -- maybe even extend that to 48 hours --
before committing.

I'd disagree here - and perhaps my motivation should be enshrined in the documentation too: I believe a review approval is roughly equivalent to "I would be comfortable committing this code myself with post-commit review", so a single approval under those constraints should be able to be committed immediately & if other reviewers chime in later it can be handled as it would be with any other post-commit review feedback (which can include reverting).

If there are "approvals" that aren't from people who have the authority/experience to commit such a change directly, that's problematic. (& it does happen, and it is super vague about who has such authority and who doesn't, etc - but I think for most reviews "if you wrote this yourself, would you commit it without pre-commit review" should be the bar for approval (once you get up to more nuanced design discussions, that's different - and it can sometimes invert "I'd commit this without review, but I'm curious what others think/if they have better ideas" (at which point it's really more up to the submitter to make the judgment call whether they've got sufficient feedback to help them decide that there aren't better ideas out there - sometimes that comes quickly in terms of a single other main developer saying "oh, yeah, that makes total sense - here are 3 other ideas that come to mind and why they don't seem better than this" or might be a more ongoing discussion))
 
Of course, this is usually not an issue in LLVM today because we're so
bad at reviewing code in general :) If you've waited on a review for a
month and pinged people several times, then by all means feel free to
commit immediately after getting a LGTM.

Cheers,
Nicolai



>
> Thoughts?
>
>  -Hal
>
>
>
> James
>
> On Sat, 16 Nov 2019 at 16:37, 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.  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
>
> --
> 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



--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
Nicolai Hähnle via llvm-dev <[hidden email]> writes:

> Of course, this is usually not an issue in LLVM today because we're so
> bad at reviewing code in general :) If you've waited on a review for a
> month and pinged people several times, then by all means feel free to
> commit immediately after getting a LGTM.

I feel like something should be spelled out about what to do if no
responses are given to a patch.  I've had that happen multiple times,
pinging once a week for a month or more.  What is the author supposed to
do?

                        -David
_______________________________________________
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

Jeremy Morse via llvm-dev


On Wed, Nov 20, 2019 at 2:06 PM David Greene via llvm-dev <[hidden email]> wrote:
Nicolai Hähnle via llvm-dev <[hidden email]> writes:

> Of course, this is usually not an issue in LLVM today because we're so
> bad at reviewing code in general :) If you've waited on a review for a
> month and pinged people several times, then by all means feel free to
> commit immediately after getting a LGTM.

I feel like something should be spelled out about what to do if no
responses are given to a patch.  I've had that happen multiple times,
pinging once a week for a month or more.  What is the author supposed to
do?

Basically that - but yeah, it should be written down & maybe an SLA of some kind of "hey, at least reply to someone and acknowledge it's there/explain why review is slow, etc" - but you can also escalate to some extent, by trying other reviewers, looking for broader code owners if there are any, etc.
 

                        -David
_______________________________________________
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

Jeremy Morse via llvm-dev
+1 to Hal's wording, and Philip's ("A couple of additional things: ...") and James' suggestions ("If multiple reviewers have been actively reviewing the patch, ...")

"How to find appropriate reviewers" should probably also be mentioned.

On Wed, Nov 20, 2019 at 2:23 PM David Blaikie via llvm-dev <[hidden email]> wrote:


On Wed, Nov 20, 2019 at 2:06 PM David Greene via llvm-dev <[hidden email]> wrote:
Nicolai Hähnle via llvm-dev <[hidden email]> writes:

> Of course, this is usually not an issue in LLVM today because we're so
> bad at reviewing code in general :) If you've waited on a review for a
> month and pinged people several times, then by all means feel free to
> commit immediately after getting a LGTM.

I feel like something should be spelled out about what to do if no
responses are given to a patch.  I've had that happen multiple times,
pinging once a week for a month or more.  What is the author supposed to
do?

Basically that - but yeah, it should be written down & maybe an SLA of some kind of "hey, at least reply to someone and acknowledge it's there/explain why review is slow, etc" - but you can also escalate to some extent, by trying other reviewers, looking for broader code owners if there are any, etc.
 

                        -David
_______________________________________________
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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
On Wed, Nov 20, 2019 at 6:53 PM David Blaikie <[hidden email]> wrote:
> On Wed, Nov 20, 2019 at 2:28 AM Nicolai Hähnle via llvm-dev <[hidden email]> wrote:
>> On a somewhat related note: Due to timezones, I think good etiquette
>> is to wait a reasonable time before committing a patch if you receive
>> a LGTM very quickly. I think non-trivial patches should generally be
>> up for at least 24 hours -- maybe even extend that to 48 hours --
>> before committing.
>
>
> I'd disagree here - and perhaps my motivation should be enshrined in the documentation too: I believe a review approval is roughly equivalent to "I would be comfortable committing this code myself with post-commit review", so a single approval under those constraints should be able to be committed immediately & if other reviewers chime in later it can be handled as it would be with any other post-commit review feedback (which can include reverting).

Post-commit review should emphatically not be the default. Also, the
social barrier to just unilaterally reverting somebody else's commit
are high, and for a good reason.

Just because two people are comfortable committing code (the author
plus a single reviewer), that does *not* necessarily mean that the
code is good to go in as-is. Consider that both people may be from the
same team, for example. Having a cool-down period is a necessary
antidote against toxic clique behavior.

(This argument may not apply to specific subsystems. For example, if
you're maintaining a small backend that is genuinely only worked on by
a closely-knit team, then it is of course reasonable to work under
more relaxed rules.)

Cheers,
Nicolai

--
Lerne, wie die Welt wirklich ist,
aber vergiss niemals, wie sie sein sollte.
_______________________________________________
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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


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
Reply | Threaded
Open this post in threaded view
|

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

Jeremy Morse via llvm-dev
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

Jeremy Morse via llvm-dev

yes, I remember marveling at how dblaikie and echristo would have occasionally boisterous debates on-list when they essentially (possibly literally) sat next to each other.

 

When my team started participating upstream, we were advised to try to avoid getting approvals from each other, and specifically seek out reviewers from other orgs.  Partly this is a matter of perspective, in that what’s good for one org and what’s good for the upstream project may be fairly different, and that may be clearer to someone from a different org.  I also think if a patch goes through an internal review step before being posted upstream (a process we employ a fair amount in my team) then it’s best if someone else does the upstream review, just to avoid the rubber-stamp effect.  I don’t know that it’s necessary to codify this too strongly, it’s more of a how-to-pick-reviewers topic than how-we-do-reviews.

 

It would be more helpful if the Code Reviews section stated a goal (or short goal set) for the review.  People can have radically different ideas about the purpose of a code review.  I attended a software-engineering conference talk once where Google people stated flat out that the sole purpose of their internal reviews is readability.  Readability is nice, it fosters maintainability which is certainly a good thing; but it goes against my entire career’s notion that reviews primarily seek out logic holes and defects.

--paulr

 

 

From: llvm-dev <[hidden email]> On Behalf Of David Blaikie via llvm-dev
Sent: Monday, December 2, 2019 10:56 AM
To: Mehdi AMINI <[hidden email]>
Cc: [hidden email]
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

 

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

Jeremy Morse via llvm-dev


On Mon, Dec 2, 2019 at 8:36 AM Robinson, Paul <[hidden email]> wrote:

yes, I remember marveling at how dblaikie and echristo would have occasionally boisterous debates on-list when they essentially (possibly literally) sat next to each other.

 

When my team started participating upstream, we were advised to try to avoid getting approvals from each other, and specifically seek out reviewers from other orgs.  Partly this is a matter of perspective, in that what’s good for one org and what’s good for the upstream project may be fairly different, and that may be clearer to someone from a different org.  I also think if a patch goes through an internal review step before being posted upstream (a process we employ a fair amount in my team) then it’s best if someone else does the upstream review, just to avoid the rubber-stamp effect.  I don’t know that it’s necessary to codify this too strongly, it’s more of a how-to-pick-reviewers topic than how-we-do-reviews.

 

It would be more helpful if the Code Reviews section stated a goal (or short goal set) for the review.  People can have radically different ideas about the purpose of a code review.  I attended a software-engineering conference talk once where Google people stated flat out that the sole purpose of their internal reviews is readability. 


The message was maybe not convey clearly? Google's internal review require two kinds of approval and one of them is purely for "readability", and not everyone can give this approval (you need to get enough of your own patches through first).
In my experience reviews are fairly aligned with Google's public documentation on the topic: https://google.github.io/eng-practices/review/ (which I find to be a pretty good document by the way).

-- 
Mehdi
 

Readability is nice, it fosters maintainability which is certainly a good thing; but it goes against my entire career’s notion that reviews primarily seek out logic holes and defects. 

--paulr

 

 

From: llvm-dev <[hidden email]> On Behalf Of David Blaikie via llvm-dev
Sent: Monday, December 2, 2019 10:56 AM
To: Mehdi AMINI <[hidden email]>
Cc: [hidden email]
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

 

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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev
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.
 
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
Reply | Threaded
Open this post in threaded view
|

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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev

Regarding the talk I heard, Mehdi asked:

The message was maybe not convey clearly?

 

See “Modern Code Review: A Case Study at Google,” by Caitlin Sadowski et al, 40th ICSE (2018), DOI: 10.1145/3183519.3183525

In particular, Finding 1 says that readability/maintainability is the primary focus. Defect detection is clearly secondary.

I spoke to Caitlin after the paper presentation to verify this point.  But she said readability really is the primary goal, not defect detection.

 

If the paper does not reflect your perception of how review actually works, well, that may be a problem with the paper, or perhaps practices vary across the company; but I suggest you check with Caitlin or one of her co-authors first.  I can only report what Google authors reported.

 

In any case, the link you cited is a nice, short list and a worthwhile starting point for a set of LLVM code-review goals.

--paulr

 

From: Mehdi AMINI <[hidden email]>
Sent: Monday, December 2, 2019 11:49 AM
To: Robinson, Paul <[hidden email]>
Cc: David Blaikie <[hidden email]>; [hidden email]
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

 

 

 

On Mon, Dec 2, 2019 at 8:36 AM Robinson, Paul <[hidden email]> wrote:

yes, I remember marveling at how dblaikie and echristo would have occasionally boisterous debates on-list when they essentially (possibly literally) sat next to each other.

 

When my team started participating upstream, we were advised to try to avoid getting approvals from each other, and specifically seek out reviewers from other orgs.  Partly this is a matter of perspective, in that what’s good for one org and what’s good for the upstream project may be fairly different, and that may be clearer to someone from a different org.  I also think if a patch goes through an internal review step before being posted upstream (a process we employ a fair amount in my team) then it’s best if someone else does the upstream review, just to avoid the rubber-stamp effect.  I don’t know that it’s necessary to codify this too strongly, it’s more of a how-to-pick-reviewers topic than how-we-do-reviews.

 

It would be more helpful if the Code Reviews section stated a goal (or short goal set) for the review.  People can have radically different ideas about the purpose of a code review.  I attended a software-engineering conference talk once where Google people stated flat out that the sole purpose of their internal reviews is readability. 

 

The message was maybe not convey clearly? Google's internal review require two kinds of approval and one of them is purely for "readability", and not everyone can give this approval (you need to get enough of your own patches through first).

In my experience reviews are fairly aligned with Google's public documentation on the topic: https://google.github.io/eng-practices/review/ (which I find to be a pretty good document by the way).

 

-- 

Mehdi

 

Readability is nice, it fosters maintainability which is certainly a good thing; but it goes against my entire career’s notion that reviews primarily seek out logic holes and defects. 

--paulr

 

 

From: llvm-dev <[hidden email]> On Behalf Of David Blaikie via llvm-dev
Sent: Monday, December 2, 2019 10:56 AM
To: Mehdi AMINI <[hidden email]>
Cc: [hidden email]
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

 

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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


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. 

 
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
Reply | Threaded
Open this post in threaded view
|

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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


On Mon, Dec 2, 2019 at 11:36 AM Robinson, Paul <[hidden email]> wrote:

yes, I remember marveling at how dblaikie and echristo would have occasionally boisterous debates on-list when they essentially (possibly literally) sat next to each other.

 

When my team started participating upstream, we were advised to try to avoid getting approvals from each other, and specifically seek out reviewers from other orgs.  Partly this is a matter of perspective, in that what’s good for one org and what’s good for the upstream project may be fairly different, and that may be clearer to someone from a different org.  I also think if a patch goes through an internal review step before being posted upstream (a process we employ a fair amount in my team) then it’s best if someone else does the upstream review, just to avoid the rubber-stamp effect.  I don’t know that it’s necessary to codify this too strongly, it’s more of a how-to-pick-reviewers topic than how-we-do-reviews.


Yeah, it helps to do as much of that upstream as possible - even if it's between two people from the same org. It helps upstream understand what sort of scrutiny has already been applied to the patch (& allows others to pipe up if it might be going down a path they don't agree with/might have suggestions about)

Also there's a certain amount of the code ownership issue - & I /think/ that's more what came up (in my mind) with some contributions in the past: Two relative newcomers to the project approving each others patches. That to me is usually addressed by the "don't approve somtehing you wouldn't've committed yourself without review" - except at the code owner level where owners/frequent contributors might be seeking feedback from each other on design decisions with no clear hierarchy between them. But I realize that gets a bit fuzzy/awkward.
 

 

It would be more helpful if the Code Reviews section stated a goal (or short goal set) for the review.  People can have radically different ideas about the purpose of a code review.  I attended a software-engineering conference talk once where Google people stated flat out that the sole purpose of their internal reviews is readability.  Readability is nice, it fosters maintainability which is certainly a good thing; but it goes against my entire career’s notion that reviews primarily seek out logic holes and defects.

--paulr

 

 

From: llvm-dev <[hidden email]> On Behalf Of David Blaikie via llvm-dev
Sent: Monday, December 2, 2019 10:56 AM
To: Mehdi AMINI <[hidden email]>
Cc: [hidden email]
Subject: Re: [llvm-dev] [RFC] High-Level Code-Review Documentation Update

 

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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev

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?

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

Jeremy Morse via llvm-dev


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. 

- 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

Jeremy Morse via llvm-dev


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. 
- 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

Jeremy Morse via llvm-dev
In reply to this post by Jeremy Morse via llvm-dev


On Mon, Dec 2, 2019 at 10:05 AM David Blaikie <[hidden email]> 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'm aligned with David on this, and this sentence summarizes fairly well my view as well.
I wasn't trying to point at the exception: I just never saw "belonging to the same organization" as a very important discriminator in our review process. I pick the best reviewer regardless, and I review patches in my area similarly.

Best,

-- 
Mehdi

 
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. 

- 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
123