Deprecating llvm::Optional<X>::hasValue/getValue/getValueOr

LLVM Project
26

I don’t think we can use operator* here, as there is nothing guaranteeing it asserts.

I’m sure I’m not the only developer who has historically used llvm::Optional::operator* without an additional assert because it was defined to do the assert internally. This implies that just swapping in std::optional for llvm::Optional is equivalent to deleting a lot of important asserts.

As an example of a change making this mistake see 467432899bc2f71842ed1b24d24c094da02af7d4 “MemoryLocation: convert Optional to std::optional”. Before that change, this code would always assert in get if it saw None:

static MemoryLocation get(const Instruction *Inst) {
  return *MemoryLocation::getOrNone(Inst);
  //     ^ Guaranteed to assert(hasValue())
}
static Optional<MemoryLocation> getOrNone(const Instruction *Inst);

The change should have retained the assert one way or another. Assuming the value method is an issue on some platforms, the most straightforward option is to add a temporary and use a plain assert:

static MemoryLocation get(const Instruction *Inst) {
  auto ML = MemoryLocation::getOrNone(Inst);
  assert(ML);
  return *ML;
}
static std::optional<MemoryLocation> getOrNone(const Instruction *Inst);

I think this harms readability on several fronts (stretches vertically, breaks up expressions needlessly, and forces the reader to consider the possibility that the temporary variable has some other purpose later in the scope), so I want to propose something I think is an improvement over the original llvm::Optional version: a Rust-like unwrap/expect, but implemented as a free-function. Then the above becomes:

static MemoryLocation get(const Instruction *Inst) {
  return unwrap(MemoryLocation::getOrNone(Inst));
}
static std::optional<MemoryLocation> getOrNone(const Instruction *Inst);

Bikeshedding on naming is welcome, but the point is just to have an assert-and-then-dereference style function that can be used uniformly for these cases. It could be as simple as a new header llvm/ADT/Unwrap.h with a single function:

namespace llvm {

/// Assert the truthiness of, and dereference, fallible
/// wrapper-like object \p C.
template <typename T>
inline auto unwrap(T &&Wrapper) -> decltype(*std::forward<T>(Wrapper)) {
  assert(Wrapper);
  return *std::forward<T>(Wrapper);
}

} // namespace llvm

For cases trivially dominated by a check we could still safely allow the bare dereference operator:

std::optional<int> I;
if (I)
  return *I; // Fine

Thoughts?