Page MenuHomePhabricator

Make mlir::Value's bool conversion operator explicit
ClosedPublic

Authored by bkramer on May 24 2020, 2:12 PM.

Details

Summary

This still allows if (value) while requiring an explicit cast when not
in a boolean context. This means things like std::set<Value> will no
longer compile.

Diff Detail

Event Timeline

bkramer created this revision.May 24 2020, 2:12 PM
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
bkramer updated this revision to Diff 265944.May 24 2020, 2:13 PM

Put back sort

Harbormaster completed remote builds in B57757: Diff 265943.
mehdi_amini accepted this revision.May 24 2020, 5:06 PM
This revision was not accepted when it landed; it landed in state Needs Review.May 25 2020, 9:38 AM
This revision was automatically updated to reflect the committed changes.

@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).
@rriddle - would you mind not setting your Herald rule to "requires review" unless you plan to review everything that triggers that rule - otherwise, again, it ends up looking like a patch was committed without the intended scrutiny.

@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.

@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)

ftynse added a subscriber: ftynse.May 28 2020, 4:28 AM

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.

@ftynse This would suggest an update to this part of the docs then, right? https://llvm.org/docs/Phabricator.html#reviewing-code-with-phabricator

@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.

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.

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.

Perhaps I should enrich the sentence set.

// ==UserScript==
// @name         reviews.llvm.org: add comment "LGTM" when accepting
// @namespace    http://tampermonkey.net/
// @version      0.1
// @description  try to take over the world!
// @author       You
// @match        https://reviews.llvm.org/*
// @grant        none
// ==/UserScript==

(function() {
  'use strict';
  const select = document.querySelector('.aphront-form-input > select')
  if (select)
    select.onchange = function() {
      const accept = select.querySelector('option[value=accept]')
      if (accept && accept.disabled) {
        const comment = document.querySelector('.remarkup-assist-textarea')
        if (comment && comment.value.length == 0) {
          const words = ['LGTM.', 'Thanks!', 'Looks great!']
          comment.value = words[~~(Math.random()*words.length)]+' '
          comment.focus()
        }
      }
    }
})();

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).

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.

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 (

Not anymore: I have seen more and more reviews created for the sake of having the pre merge testing.

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 (

Not anymore: I have seen more and more reviews created for the sake of having the pre merge testing.

If those are going to the mailing list - I don't think that's appropriate (it's noise for an already very busy mailing list, and it muddies the water about what threads need review and which ones are to be ignored). & if they don't go to the mailing list, I think that's fine - they're not reviews as far as I'm concerned.

I believe that since we moved to the monorepo, the mailing-lists are added automatically based on the path touched in the reviews

I believe that since we moved to the monorepo, the mailing-lists are added automatically based on the path touched in the reviews

Yeah, I think someone figured out how to make that happen, maybe even before the monorepo/a while back - and I stand by the position that I don't think it's appropriate to send mail to the mailing list for automated pre-commit testing. If it's not possible to use Phab in its current configuration to get the testing without sending email, please fix/improve phab to allow that before using it for non-reviewed pre-commit testing. I'll certainly keep an eye out for it & discourage the practice.

mehdi_amini added a comment.EditedMay 31 2020, 10:08 AM

Why?
Given the tradeoff between breaking the build in master for many or having an extra email sent: I pick the latter without hesitation!

Why?
Given the tradeoff between breaking the build in master for many or having an extra email sent: I pick the latter without hesitation!

Different tradeoffs, it seems. Though those using the pre-commit checking could work on/make changes to Phabricator to enable pre-commit checking without sending email.

At least in the Web UI there's a way to change who a review is visible to? Perhaps you can make it just visible to yourself & maybe it'll still run the precommit checks but not send any email? (seems making it non-visible might thwart the pre-release testing herald rule, perhaps... )

Hey, looks like someone already added something to the LLVM herald rule, at least, not sure about the other subproject herald rules "Revision title does not contain [private]". So if you want an llvm commit to get precommit testing without sending llvm-dev mail, include "[private]" in the title. Other subprojects/mailing lists/rules could do similarly (I doubt there's any way we could make a general rule that overrides all the others... ) - though, I imagine,

I fixed the broken rule that wasn't matching MLIR revision, letting them go in the "catch all" llvm-commit subscription: they won't go to llvm-commit@ anymore.

Herald added a subscriber: Restricted Project. · View Herald TranscriptMay 31 2020, 11:54 AM

I fixed the broken rule that wasn't matching MLIR revision, letting them go in the "catch all" llvm-commit subscription: they won't go to llvm-commit@ anymore.

As in: none-of-them will go to llvm-commits@, it does not address our difference of expectation about priority, but at least we're getting out of your mailbox :)