<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Dec 2, 2019 at 12:52 PM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
  
    
  
  <div>
    <p>Mehdi, David,</p>
    <p>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.</p>
    <p>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?</p></div></blockquote><div><br>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. <br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>
    <p>Philip<br>
    </p>
    <div>On 12/2/19 7:55 AM, David Blaikie
      wrote:<br>
    </div>
    <blockquote type="cite">
      
      <div dir="ltr">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... :/ )))</div>
      <br>
      <div class="gmail_quote">
        <div dir="ltr" class="gmail_attr">On Wed, Nov 27, 2019 at 10:27
          PM Mehdi AMINI via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>
          wrote:<br>
        </div>
        <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
          <div dir="ltr">
            <div dir="ltr">
              <div><br>
              </div>
              <br>
              <div class="gmail_quote">
                <div dir="ltr" class="gmail_attr">On Sat, Nov 16, 2019
                  at 5:56 PM Mehdi AMINI <<a href="mailto:joker.eph@gmail.com" target="_blank">joker.eph@gmail.com</a>>
                  wrote:<br>
                </div>
                <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
                  <div dir="ltr">+1 in general, and Philip has good
                    suggestions as well!<br>
                    <div><br>
                    </div>
                    <div>-- </div>
                    <div>Mehdi</div>
                    <div><br>
                    </div>
                  </div>
                  <br>
                  <div class="gmail_quote">
                    <div dir="ltr" class="gmail_attr">On Sat, Nov 16,
                      2019 at 8:37 AM Philip Reames via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>
                      wrote:<br>
                    </div>
                    <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">+ 1 in general,
                      a couple of suggestions<br>
                      <br>
                      On 11/14/19 7:46 PM, Finkel, Hal J. via llvm-dev
                      wrote:<br>
                      > Hi, everyone,<br>
                      ><br>
                      > I've been fielding an increasing number of
                      questions about how our <br>
                      > code-review process in LLVM works from people
                      who are new to our <br>
                      > community, and it's been pointed out to me
                      that our documentation on <br>
                      > code reviews is both out of date and not as
                      helpful as it could be to <br>
                      > new developers.<br>
                      ><br>
                      >    <a href="http://llvm.org/docs/DeveloperPolicy.html#code-reviews" rel="noreferrer" target="_blank">http://llvm.org/docs/DeveloperPolicy.html#code-reviews</a><br>
                      ><br>
                      > I would like to compose a patch to update
                      this, but before I do that, I <br>
                      > want to highlight some of my thoughts to get
                      feedback. My intent is to <br>
                      > capture our community best practices in
                      writing so that people new to <br>
                      > our community understand our processes and
                      expectations. Here are some <br>
                      > things that I would like to capture:<br>
                      ><br>
                      >   1. You do not need to be an expert in some
                      area of the compiler to <br>
                      > review patches; it's fine to ask questions
                      about what some piece of code <br>
                      > is doing. If it's not clear to you what is
                      going on, you're unlikely to <br>
                      > be the only one. Extra comments and/or test
                      cases can often help (and <br>
                      > asking for comments in the test cases is fine
                      as well).<br>
                      Authors are encouraged to interpret questions as
                      reasons to reexamine<br>
                      the readability of the code in question. 
                      Structural changes, or further<br>
                      comments may be appropriate.<br>
                      ><br>
                      >   2. If you review a patch, but don't intend
                      for the review process to <br>
                      > block on your approval, please state that
                      explicitly. Out of courtesy, <br>
                      > we generally wait on committing a patch until
                      all reviewers are <br>
                      > satisfied, and if you don't intend to look at
                      the patch again in a <br>
                      > timely fashion, please communicate that fact
                      in the review.<br>
                      ><br>
                      >   3. All comments by reviewers should be
                      addressed by the patch author. <br>
                      > It is generally expected that suggested
                      changes will be incorporated <br>
                      > into the next revision of the patch unless
                      the author and/or other <br>
                      > reviewers can articulate a good reason to do
                      otherwise (and then the <br>
                      > reviewers must agree). If you suggest changes
                      in a code review, but <br>
                      > don't wish the suggestion to be interpreted
                      this strongly, please state <br>
                      > so explicitly.<br>
                      ><br>
                      >   4. Reviewers may request certain aspects of
                      a patch to be broken out <br>
                      > into separate patches for independent review,
                      and also, reviewers may <br>
                      > accept a patch conditioned on the author
                      providing a follow-up patch <br>
                      > addressing some particular issue or concern
                      (although no committed patch <br>
                      > should leave the project in a broken state).
                      Reviewers can also accept a <br>
                      > patch conditioned on the author applying some
                      set of minor updates prior <br>
                      > to committing, and when applicable, it is
                      polite for reviewers to do so.<br>
                      ><br>
                      >   5. Aim to limit the number of iterations in
                      the review process. For <br>
                      > example, when suggesting a change, if you
                      want the author to make a <br>
                      > similar set of changes at other places in the
                      code, please explain the <br>
                      > requested set of changes so that the author
                      can make all of the changes <br>
                      > at once. If a patch will require multiple
                      steps prior to approval (e.g., <br>
                      > splitting, refactoring, posting data from
                      specific performance tests), <br>
                      > please explain as many of these up front as
                      possible. This allows the <br>
                      > patch author to make the most-efficient use
                      of his or her time.<br>
                      If the path forward is not clear - because the
                      patch is too large to<br>
                      meaningful review, or direction needs to be
                      settled - it is fine to<br>
                      suggest a clear next step (e.g. landing a
                      refactoring) followed by a<br>
                      re-review.  Please state explicitly if the path
                      forward is unclear to<br>
                      prevent confusions on the part of the author. <br>
                      ><br>
                      >   6. Some changes are too large for just a
                      code review. Changes that <br>
                      > should change the Language Reference (e.g.,
                      adding new <br>
                      > target-independent intrinsics), adding
                      language extensions in Clang, and <br>
                      > so on, require an RFC on *-dev first. For
                      changes that promise <br>
                      > significant impact on users and/or downstream
                      code bases, reviewers can <br>
                      > request an RFC (Request for Comment)
                      achieving consensus before <br>
                      > proceeding with code review. That having been
                      said, posting initial <br>
                      > patches can help with discussions on an RFC.<br>
                      ><br>
                      > Lastly, the current text reads, "Code reviews
                      are conducted by email on <br>
                      > the relevant project’s commit mailing list,
                      or alternatively on the <br>
                      > project’s development list or bug tracker.",
                      and then only later <br>
                      > mentions Phabricator. I'd like to move
                      Phabricator to be mentioned on <br>
                      > this line before the other methods.<br>
                      ><br>
                      > Please let me know what you think.<br>
                      ><br>
                      > Thanks again,<br>
                      ><br>
                      > Hal<br>
                      <br>
                      A couple of additional things:<br>
                      <br>
                      Only a single LGTM is required.  Reviewers are
                      expected to only LGTM<br>
                      patches they're confident in their knowledge of. 
                      Reviewers may review<br>
                      and provide suggestions, but explicitly defer LGTM
                      to someone else. <br>
                      This is encouraged and a good way for new
                      contributors to learn the code. <br>
                      <br>
                      There is a cultural expectation that at least one
                      reviewer is from a<br>
                      different organization than the author of the
                      patch. </blockquote>
                  </div>
                </blockquote>
                <div><br>
                </div>
                <div>Actually, while I'm OK with the other suggestions,
                  I didn't pay attention to this one originally.</div>
                <div>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.</div>
                <div><br>
                </div>
                <div>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.</div>
                <div>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).</div>
                <div><br>
                </div>
                <div>-- </div>
                <div>Mehdi</div>
                <div><br>
                </div>
                <div><br>
                </div>
                <div><br>
                </div>
                <div> </div>
                <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
                  <div class="gmail_quote">
                    <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> If that's not<br>
                      possible, care should be taken to ensure overall
                      direction has been<br>
                      widely accepted. <br>
                      <br>
                      Post commit review is encouraged via either
                      phabricator or email.  There<br>
                      is a strong expectation that authors respond
                      promptly to post commit<br>
                      feedback and address it.  Failure to do so is
                      cause for the patch to be<br>
                      reverted.  If substantial problems are identified,
                      it is expected that<br>
                      the patch is reverted, fixed offline, and then
                      recommitted (possibly<br>
                      after further review.)<br>
                      <br>
                      <br>
                      _______________________________________________<br>
                      LLVM Developers mailing list<br>
                      <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
                      <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
                    </blockquote>
                  </div>
                </blockquote>
              </div>
            </div>
          </div>
          _______________________________________________<br>
          LLVM Developers mailing list<br>
          <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a><br>
          <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
        </blockquote>
      </div>
    </blockquote>
  </div>

</blockquote></div></div>