llvm dev - Aug 2020 - 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?

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

On Sun, Aug 16, 2020 at 4:54 PM Paul C. Anagnostopoulos <paul at
windfall.com>
wrote:
> 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?
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200816/bd9f2da3/attachment.html>

At 8/16/2020 10:58 AM, Stefan Stipanovic wrote:
>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