David Blaikie via llvm-dev
2019-Dec-02 18:05 UTC
[llvm-dev] [RFC] High-Level Code-Review Documentation Update
On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <listmail at philipreames.com> 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 < > llvm-dev at lists.llvm.org> wrote: > >> >> >> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com> 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 < >>> llvm-dev at lists.llvm.org> 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 >>>> llvm-dev at lists.llvm.org >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/22449c8e/attachment-0001.html>
Philip Reames via llvm-dev
2019-Dec-03 01:23 UTC
[llvm-dev] [RFC] High-Level Code-Review Documentation Update
On 12/2/19 10:05 AM, David Blaikie wrote:> > > On Mon, Dec 2, 2019 at 12:52 PM Philip Reames > <listmail at philipreames.com <mailto:listmail at philipreames.com>> 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 >> <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >> >> >> >> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI >> <joker.eph at gmail.com <mailto:joker.eph at gmail.com>> 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 <llvm-dev at lists.llvm.org >> <mailto:llvm-dev at lists.llvm.org>> 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 >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/34481b6d/attachment-0001.html>
Mehdi AMINI via llvm-dev
2019-Dec-03 02:21 UTC
[llvm-dev] [RFC] High-Level Code-Review Documentation Update
On Mon, Dec 2, 2019 at 10:05 AM David Blaikie <dblaikie at gmail.com> wrote:> > > On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <listmail at philipreames.com> > 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 < >> llvm-dev at lists.llvm.org> wrote: >> >>> >>> >>> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com> 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 < >>>> llvm-dev at lists.llvm.org> 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 >>>>> llvm-dev at lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>> >>>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191202/1a2fa97e/attachment.html>
David Blaikie via llvm-dev
2019-Dec-03 15:45 UTC
[llvm-dev] [RFC] High-Level Code-Review Documentation Update
On Mon, Dec 2, 2019 at 8:23 PM Philip Reames <listmail at philipreames.com> wrote:> > On 12/2/19 10:05 AM, David Blaikie wrote: > > > > On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <listmail at philipreames.com> > wrote: > >> Mehdi, David, >> >> I think you're both pointing out exceptions rather than the general >> rule. I tried to indicate their might be reasonable exceptions (see the >> second sentence past Mehdi's quote), but in general, particularly for new >> contributors, I think it is important we indicate something to this >> effect. I've seen multiple new groups have issues around this. In some >> cases, patches were reverted in post review. In others, a bunch of time >> was sunk in a direction which turned turned out not to have wide >> agreement. Cautioning folks to avoid that is important. >> >> Do you have any suggestions on wording which keep the broad message, but >> make it more clear that it isn't a hard and fast rule? >> > > My take on it is that even the broad message isn't how I think of LLVM > code review practices - if the code owner/domain expert is at your company, > so be it. If not, that's fine too - I see lots of reviews by people at the > same company that generally look pretty good & I don't think their the > exception. I'd personally leave this part out - maybe some caveat about not > doing internal review & then summarily approving externally? But that I > think is more the exception than the rule & perhaps not worth including in > the general practices document. But if you've seen several instances of > this sort of issue that seem worth addressing through this mechanism - > yeah, I'd be OK with an encouragement to avoid insular project areas where > possible (especially where there's strong overlap) - seek > external/long-term contributor input especially on design documents and > early/foundational patches, and in general err on the side of seeking more > input from code owners than not. If you ask too often for trivial things, > that's fine - they should hopefully get to the point where they encourage > you to contribute directly/stop asking for their review/approval. But > especially when asking code owners/frequent reviewers/contributors for > review, be extra sure to make the patches small and clear, to have design > discussion ahead of time to avoid designing in a large unwieldy code > review, etc. > > Hm, I see your point, but I'm not 100% in agreement. I'm trying to find > the hair to split, but I'm struggling to find the right one. Let's drop > this for the moment from the initial patch, and I'll try to a rework on the > wording in a separate review. I don't want to risk further derailing the > overall effort right now. >Fair - certainly up for chatting about it further to try to come to some common understanding/phrasing/etc.> - Dave > > >> Philip >> On 12/2/19 7:55 AM, David Blaikie wrote: >> >> Yeah, +1 that people from the same organization are sometimes the only >> ones working on a certain feature/area. (certainly I'd expect some >> discussion about the feature in general to be discussed outside that group >> if it's in any way contentious - but some stuff's clear enough (I think I >> implemented debug_types years ago, likely with only Eric's approval, both >> of us being at Google (probably many DWARF features were added/done this >> way, to be honest - maybe some could've done witha bit of broader >> discussion, but I don't think either of us were "rubber stamping" the >> other's work (if anything I'm harder on my "friends" to be honest... :/ ))) >> >> On Wed, Nov 27, 2019 at 10:27 PM Mehdi AMINI via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> >>> >>> On Sat, Nov 16, 2019 at 5:56 PM Mehdi AMINI <joker.eph at gmail.com> 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 < >>>> llvm-dev at lists.llvm.org> 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 >>>>> llvm-dev at lists.llvm.org >>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>>> >>>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20191203/3e44fc6a/attachment-0001.html>