[llvm-dev] Policy question about Phabricator reviews

[Simon Pilgrim via llvm-dev]

Hi Paul,

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

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
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev