[PATCH] D130645: Move mlir LogicalResult to llvm Support library

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Support/LogicalResult.h:96
   /// Hide the bool conversion as it easily creates confusion.
   using Optional<T>::operator bool;
   using Optional<T>::has_value;
----------------
rriddle wrote:
> dblaikie wrote:
> > rriddle wrote:
> > > dblaikie wrote:
> > > > rriddle wrote:
> > > > > ezhulenev wrote:
> > > > > > dblaikie wrote:
> > > > > > > Seems unfortunate that this type (`FailureOr`) will have different API than `ErrorOr` though - any chance this could keep an explicit operator bool?
> > > > > > > 
> > > > > > > & making something private that's inherited is pretty awkward - since you can still access the private things via a base reference, or slicing, etc.
> > > > > > > 
> > > > > > > I guess alternatively, maybe `ErrorOr` ends up with `operator LogicalResult` instead of `explicit operator bool`? (@lhames )
> > > > > > If I got the history correctly that's a very deliberate decision (cc @rriddle), only the `mlir::ParseResult` allows implicit casting to bool, because it's intended for use in a very narrow context of parsing. Because if `FailureOr` would have implicit casting it would be another source of confusion, I myself always double check the rules for casting `llvm::Error` and `llvm::Expected`.
> > > > > Yeah, no conversions to bool was an explicit choice. Alongside LogicalResult, we wanted something that is explicit about bool treatment.
> > > > > 
> > > > > The choice of using Optional is the base class was just convenience, we could change to something cleaner.
> > > > Oh, no doubt it was deliberate - but I guess to summarize: given the similarities with Error/ErrorOr, it seems important these have similar/the same API in this regard. So I think it's important to at least have a plan everyone agrees to about how these'll all end up the same in relatively short order before this gets added to LLVM. (& ideally, avoid even temporary divergence if practical/if some of it can be frontloaded (I guess that'd involve introducing LogicalResult first, using that in ErrorOr, then introducing FailureOr at that point since it would be consistent at that point))
> > > I don't think FailureOr should have any kind of boolean conversion (whether implicit/explicit). I'd be fine with aligning on the other API though, of which it only seems like ErrorOr has operator*/operator->/get that would be shared (there doesn't seem to be any other non-Error related API on ErrorOr). Given the use of Optional, they are already kind of aligned there. I suppose to make them truly aligned, we'd need to drop various methods inherited from Optional.
> > Mostly the boolean conversion I think should be consistent between them - both or neither. It'd be pretty awkward if we have lots of code that does:
> > ```
> > if (ErrorOr<T> T = func())
> > ```
> > (which we do)
> > and then `FailureOr` can't be used in a similar way - that'll be pretty jarring for developers to have to handle these two things so fundamentally differently. I think this usage is pretty important & it'd be a loss to break all that `ErrorOr` usage - so I hope there's some way to align these while preserving such usage, but that ultimately (I think) boils down to at least an explicit conversion to bool. Which would mean we're at an impass.
> The point of the LogicalResult stuff is to not have boolean conversions. If the requirement for moving into LLVM Support is to have them, I suppose we shouldn't pull any of this into LLVM.
For `FailureOr` in particular, though - what're the risks of explicit conversion to bool that the absence is intended to mitigate?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130645