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

[Alex Zinenko via Phabricator via llvm-commits]

ftynse added a comment.

> 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 one starts with an inverse assumption "anything that went through phabricator was reviewed and approved by relevant people before being committed", 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. 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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80497