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

[River Riddle via Phabricator via llvm-commits]

rriddle 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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130645