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

[Eugene Zhulenev via Phabricator via llvm-commits]

ezhulenev 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;
----------------
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`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130645