Paul C. Anagnostopoulos via llvm-dev
2020-Aug-16 14:26 UTC
[llvm-dev] 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?
Stefan Stipanovic via llvm-dev
2020-Aug-16 14:39 UTC
[llvm-dev] Policy question about Phabricator reviews
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 On Sun, Aug 16, 2020 at 4:29 PM Paul C. Anagnostopoulos via llvm-dev < llvm-dev at lists.llvm.org> 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 >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200816/406760e6/attachment.html>
Simon Pilgrim via llvm-dev
2020-Aug-16 14:48 UTC
[llvm-dev] Policy question about Phabricator reviews
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
Paul C. Anagnostopoulos via llvm-dev
2020-Aug-16 14:51 UTC
[llvm-dev] Policy question about Phabricator reviews
At 8/16/2020 10:39 AM, Stefan Stipanovic wrote:>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)Aha! I didn't see the green Accepted indicator in the upper-left. I should look around more carefully. --------------------------------------------->Â 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.It's a new document and the post-acceptance review is reasonably substantial AND by a different reviewer. I should have made that clear in my first post. Do you think I should incorporate the other reviewer's comments first?