On Mon, May 17, 2021 at 11:12 AM Krzysztof Parzyszek via cfe-dev < cfe-...@lists.llvm.org> wrote:
> This is a revision of the previous RFC[1]. This RFC limits the scope to > pre-commit reviews only. > > > > *Statement:* > > Our current code review policy states[2]: > > “Code reviews are conducted, in order of preference, on our web-based > code-review tool (see Code Reviews with Phabricator), by email on the > relevant project’s commit mailing list, on the project’s development list, > or on the bug tracker.” > > This proposal is to limit pre-commit code reviews only to Phabricator. > This would apply to all projects in the LLVM monorepo. With the change in > effect, the amended policy would read: > > “Pre-commit code reviews are conducted on our web-based code-review tool > (see Code Reviews with Phabricator). > I'm with you here ^, this seems to document/formalize existing practice - though does this accurately reflect all the projects in the mororepo? I get the impression that mlir, maybe flang, etc might be doing reviews differently? > Post-commit reviews are conducted, in order of preference, on Phabricator, > This still seems like a change in practice that I'm not in favor of, personally - due to the current divergence between email and phab review feedback. Yes, this would be one way to unify it - but I'm not sure it's necessarily the best one. I'd suggest leaving this to a separate proposal so as not to complicate/muddy the waters of the formalization of pre-commit review practice. > by email on the relevant project’s commit mailing list, on the project’s > development list, or on the bug tracker.” > > > > *Current situation:* > > 1. In a recent llvm-dev thread[3], Christian Kühnel pointed out that > pre-commit code reviews rarely originate via an email (most are started on > Phabricator), although, as others pointed out, email responses to an > ongoing review are not uncommon. (That thread also contains examples of > mishaps related to the email-Phabricator interactions, or email handling > itself.) > 2. We have Phabricator patches that automatically apply email comments > to the Phabricator reviews, although reportedly this functionality is not > fully reliable[4,5]. This can cause review comments to be lost in the > email traffic. > > > > *Benefits:* > > 1. Single way of doing pre-commit code reviews: these code reviews are > a key part of the development process, and having one way of performing > them would make the process clearer and unambiguous. > 2. Review authors and reviewers would only need to monitor one source > of comments without the fear that a review comment may end up overlooked. > 3. This change simply codifies an existing practice. > > > > *Concerns:* > > 1. Because of the larger variety, email clients may offer better > accessibility options than web browsers. > > > > > > [1] https://lists.llvm.org/pipermail/llvm-dev/2021-May/150344.html > > [2] > https://llvm.org/docs/CodeReview.html#what-tools-are-used-for-code-review > > [3] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150129.html > > [4] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150136.html > > [5] https://lists.llvm.org/pipermail/llvm-dev/2021-April/150139.html > > > > > > -- > > Krzysztof Parzyszek kparz...@quicinc.com AI tools development > > > _______________________________________________ > cfe-dev mailing list > cfe-...@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev >
_______________________________________________ lldb-dev mailing list lldb-dev@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-dev
