[PATCH] D80497: Make mlir::Value's bool conversion operator explicit

Thu May 28 17:06:28 PDT 2020

dblaikie added a subscriber: MaskRay.
dblaikie added a comment.

In D80497#2059506 <https://reviews.llvm.org/D80497#2059506>, @mehdi_amini wrote:

> In D80497#2059323 <https://reviews.llvm.org/D80497#2059323>, @mehdi_amini wrote:
>
> > In D80497#2055817 <https://reviews.llvm.org/D80497#2055817>, @dblaikie wrote:
> >
> > > @mehdi_amini - would you mind writing some kind of comment when you approve a patch in Phabricator? Without any text in the approval, the email is not sent to the mailing list (only to the people in the review) & it looks like the patch is then committed without approval (according to the mail on the mailing list).
> >
> >
> > I don't think I can promise to remember this, this is too easy to forget to me.
>
>
> (of course I'll try, that was a disclaimer because I know myself and I'm not good at manual processes: I've been doing OK with computers because I can automate stuff)

Sure enough - appreciate the best effort.

I believe @MaskRay automated this for himself in some way - I can't find the thread right now, but I think he created some fragment of javascript using a browser trigger that automatically inserts a comment for him when approving patches - perhaps he can let you know how he managed it/you can use that.

In D80497#2059657 <https://reviews.llvm.org/D80497#2059657>, @ftynse wrote:

> FWIW, I think not having comment text associated with an approval is not a problem. If a silently approved diff lands and is auto-closed, the list receives a message "D????? was updated to reflect changes" that does _not_ contain "This revision was not accepted when it landed". The latter only appears if there are no approvals at all or if there remain any blocking reviewers. If we want to see _who_ approved the changes, maybe it's worth keeping the "reviewed-by" line in the commit message, which still wouldn't require all reviewers to remember to put "LGTM" textually when approving.

I don't agree - the absence of a "this was committed without approval" is, I think, too subtle a signal for the mailing list/all other developers to take as a clear indication that a patch was reviewed. The mailing list is meant to be the system of record here, and Phabricator is a convenient tool to facilitate using it - but review communications must be on the mailing list.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D80497/new/

https://reviews.llvm.org/D80497