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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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

> > 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.
>
> Well, it depends on the base assumption you make, which seems to be "anything is considered unreviewed and unapproved unless explicitly stated otherwise".


If it's been sent for review, yes - it's unapproved until it's... approved. That's what sending something for review is all about. If something is sent for review, then committed without it - that's a problem (

> If one starts with an inverse assumption "anything that went through phabricator was reviewed and approved by relevant people before being committed",

LLVM's review process is recorded on the mailing list - approvals need to be there. I'm not OK with subtle exceptions for Phabricator's email nuances.

> then it becomes sufficient to only check for evidence of the lacking approval. This latter approach sounds more consistent with the policy of trust that lets active contributors having direct commit access and thus being able to land unreviewed code anyway.

It isn't to me - if something is sent for review, the assumption is there was something that particularly needed a second set of eyes - if it gets committed without that, it looks like someone decided "well, I wasn't sure about this change, but since no one spoke up I'll commit it anyway" which looks like committing something that didn't meet the needed scrutiny.

>   What actually becomes invisible on the list is _who_ approved the changes, which is solvable by keeping the "reviewed-by" field in the commit message.
>    
> 
> I am generally wary of putting more manual work and cognitive overhead on reviewers.
> 
>> @ftynse This would suggest an update to this part of the docs then, right? https://llvm.org/docs/Phabricator.html#reviewing-code-with-phabricator
> 
> I prefer not to change policies without a discussion on a generally-visible channel (which isn't a review thread on a specific patch).

Agreed - I think @Kayjukh's point is that the policy states the thing that I've requested here - so, if you'd like to do something other than that, it would be good to have a discussion on llvm-dev about changing that policy. Until then, please make sure the mailing list has a record of review approvals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80497