Make mlir::Value's bool conversion operator explicit

[bkramer]

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.

[bkramer]

Put back sort

[dblaikie]

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

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

[mehdi_amini]

In D80497#2059323, @mehdi_amini wrote:

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

[ftynse]

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.

[Kayjukh]

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

[dblaikie]

In D80497#2059506, @mehdi_amini wrote:

In D80497#2059323, @mehdi_amini wrote:

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

[MaskRay]

In D80497#2061854, @dblaikie wrote:

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()
        }
      }
    }
})();

[ftynse]

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

[dblaikie]

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

[mehdi_amini]

In D80497#2064999, @dblaikie wrote:

In 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 (

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

[dblaikie]

In D80497#2065134, @mehdi_amini wrote:

In D80497#2064999, @dblaikie wrote:

In 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 (

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.

[mehdi_amini]

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

[dblaikie]

In D80497#2065221, @mehdi_amini wrote:

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]

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

[dblaikie]

In D80497#2065377, @mehdi_amini wrote:

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,

[mehdi_amini]

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.

[mehdi_amini]

In D80497#2065432, @mehdi_amini wrote:

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

mlir/include/mlir/EDSC/Builders.h

struct StructuredIndexed {
   }
   StructuredIndexed(Type t, ArrayRef<AffineExpr> indexings)
       : type(t), exprs(indexings.begin(), indexings.end()) {
     assert((t.isa<MemRefType>() || t.isa<RankedTensorType>() ||
             t.isa<VectorType>()) &&
            "MemRef, RankedTensor or Vector expected");
   }
 
-  bool hasValue() const { return value; }
+  bool hasValue() const { return (bool)value; }
   Value getValue() const {
     assert(value && "StructuredIndexed Value not set.");
     return value;
   }
   Type getType() const {
     assert((value || type) && "StructuredIndexed Value and Type not set.");
     return value ? value.getType() : type;
   }

mlir/include/mlir/IR/Value.h

public:
   template <typename U> U dyn_cast_or_null() const {
     return (*this && isa<U>()) ? U(ownerAndKind) : U(nullptr);
   }
   template <typename U> U cast() const {
     assert(isa<U>());
     return U(ownerAndKind);
   }
 
-  operator bool() const { return ownerAndKind.getPointer(); }
+  explicit operator bool() const { return ownerAndKind.getPointer(); }
   bool operator==(const Value &other) const {
     return ownerAndKind == other.ownerAndKind;
   }
   bool operator!=(const Value &other) const { return !(*this == other); }
 
   /// Return the type of this value.
   Type getType() const;
 

mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp

if (inBoundsCondition) {
       if (options.unroll)
         result = vector_insert(loaded1D, result, majorIvs);
       // 5.b. Otherwise, just go through the temporary `alloc`.
       else
         std_store(loaded1D, alloc, majorIvs);
     }
   });
 
-  assert((!options.unroll ^ result) && "Expected resulting Value iff unroll");
+  assert((!options.unroll ^ (bool)result) &&
+         "Expected resulting Value iff unroll");
   if (!result)
     result = std_load(vector_type_cast(MemRefType::get({}, vectorType), alloc));
   rewriter.replaceOp(op, result);
 
   return success();
 }
 
 template <>

mlir/lib/Parser/Parser.cpp

 }
 
 /// After parsing is finished, this function must be called to see if there are
 /// any remaining issues.
 ParseResult OperationParser::finalize() {
   // Check for any forward references that are left.  If we find any, error
   // out.
   if (!forwardRefPlaceholders.empty()) {
-    SmallVector<std::pair<const char *, Value>, 4> errors;
+    SmallVector<const char *, 4> errors;
     // Iteration over the map isn't deterministic, so sort by source location.
     for (auto entry : forwardRefPlaceholders)
-      errors.push_back({entry.second.getPointer(), entry.first});
+      errors.push_back(entry.second.getPointer());
     llvm::array_pod_sort(errors.begin(), errors.end());
 
     for (auto entry : errors) {
-      auto loc = SMLoc::getFromPointer(entry.first);
+      auto loc = SMLoc::getFromPointer(entry);
       emitError(loc, "use of undeclared SSA value name");
     }
     return failure();
   }
 
   return success();
 }