[llvm-dev] Policy question about Phabricator reviews
Simon Pilgrim via llvm-dev
llvm-dev at lists.llvm.org
Sun Aug 16 07:48:13 PDT 2020
Ideally you want the phab review to be accepted, the LGTM comment alone
is a just convention.
Sometimes reviewers leave a LGTM but with the review unaccepted to
encourage other reviewers to add their own LGTM and/or accept it as
well; accepting a review causes it to disappear from phab's "Ready to
Review" list so other reviewers might not see there's anything required
Sometimes reviewers forget to officially accept the review and just
leave a LGTM comment - if there's any uncertainty I'd recommend just
replying, asking for it to be accepted.
Often reviewers will accept and refer to a few minor required edits
(style nits etc.), in which case it should be OK to incorporate the
requested changes locally into the patch and then perform a single
commit, but if you have any uncertainty its fine to update the patch and
request reviewers review it again.
Sometimes others will only notice your patch after approval/commit and
may have post-review comments/requests, depending on the situation this
might be anything from just committing a minor change without review;
creating a followup phab patch for review; or reverting the original
commit and reopening the patch for review again. Sometimes the situation
might require someone to revert your commit and reopen your patch, in
which case they will explain the reasoning (public/downstream buildbots
breakages/regressions etc.) and will try to provide a test case - either
on the patch or on bugzilla.
Hope that helps, Simon.
On 16/08/2020 15:26, Paul C. Anagnostopoulos via llvm-dev wrote:
> I've read "LLVM Code-Review Policies and Practices," but I remain unsure of a couple of things. Do I always wait for an actual "LGTM", or can people approve the patch for submission in other ways?
> What happens when a patch is approved but then there are additional review comments? Should the patch be submitted as is, then a follow-up patch submitted, or should the additional review comments be incorporated first?
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
More information about the llvm-dev