Policy question about Phabricator reviews

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?

Hi Paul,

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?

The patch is accepted through Phabricator with or without a message (the message is usually LGTM or something similar)

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?

I think this depends on the patch. In most cases I’d incorporate the changes first and if changes are not nits, probably wait for another approval. Usually it is mentioned if you need to address comments in a follow-up.

–Stefan

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.

Aha! I didn't see the green Accepted indicator in the upper-left. I should look around more carefully.

I think you should, but if you are not sure, you can always ask for clarification in the review.

I think I will go ahead and incorporate the latest review. This is a new reference document for TableGen. There is no hurry to get it committed, since there are existing reference documents.

Thanks, folks!

~~ Paul