[ADT] Add has_value, value, value_or to llvm::Optional

[kazu]

Summary

This patch adds has_value, value, value_or to llvm::Optional so that
llvm::Optional looks more like std::optional.

I will keep the existing functions while migrating their callers and
then remove them later.

[dblaikie]

Sounds good. Though it might be worth migrating/normalising uses to op* (rather than value()) and bool testing (where it do any have to be explicit) instead of has_value() when migrating callers.

[jdoerfert]

In D128131#3594425, @dblaikie wrote:

Sounds good. Though it might be worth migrating/normalising uses to op* (rather than value()) and bool testing (where it do any have to be explicit) instead of has_value() when migrating callers.

As mentioned here by @DaniilSuchkov, and here by me, this does not always make it easier to read the code. On the contrary, this can easily lead to confusion.

As far as I can tell, there is no guideline in our coding style for these changes, as @clementval pointed out. I'd suggest a discourse post or guideline patch so we can have the discussion if this is really helpful, especially as an automatically applied code change.

[dblaikie]

In D128131#3617189, @jdoerfert wrote:

In D128131#3594425, @dblaikie wrote:

Sounds good. Though it might be worth migrating/normalising uses to op* (rather than value()) and bool testing (where it do any have to be explicit) instead of has_value() when migrating callers.

As mentioned here by @DaniilSuchkov, and here by me, this does not always make it easier to read the code. On the contrary, this can easily lead to confusion.

As far as I can tell, there is no guideline in our coding style for these changes, as @clementval pointed out. I'd suggest a discourse post or guideline patch so we can have the discussion if this is really helpful, especially as an automatically applied code change.

I don't think all cleanups need style guide wording (see a lot of @kazu's other cleanups, such as migrating to range-based algorithms and other LLVM wrappers rather than iterator-pair based standard algorithms). But yeah, totally open to some discussion about this particular one.

Not sure if it needs to go on discourse, or we can have it here - I'd be OK with a more conservative cleanup of mapping "hasValue" to "has_value" and existing implicit bool checking remains the same. (& getValue -> value() and op* remains the same.

Though I personally find bool+* to be generally easier, because it's consistent with other things like pointers, so I don't have to think about which syntax to use for this optional-ish thing. (admittedly optional<bool> is particularly hairy) - but it's not worth a big discussion/trying to make that policy change, I don't think.

@kazu - sorry for the poor advice. Would it be especially hard to fixup the already refactored code to preserve the spelling-type used (eg: migrating hasValue -> has_value, etc)? (could probably do this without actually reverting all the cleanup upstream - revert it locally, then apply a modified refactoring that preserves spelling, then squash those commits and you should have a patch that only changes the places that need to be updated to the explicit spelling?)

Would that be sufficient/good @jdoerfert ?

[jdoerfert]

In D128131#3619840, @dblaikie wrote:

In D128131#3617189, @jdoerfert wrote:

In D128131#3594425, @dblaikie wrote:

Sounds good. Though it might be worth migrating/normalising uses to op* (rather than value()) and bool testing (where it do any have to be explicit) instead of has_value() when migrating callers.

As mentioned here by @DaniilSuchkov, and here by me, this does not always make it easier to read the code. On the contrary, this can easily lead to confusion.

As far as I can tell, there is no guideline in our coding style for these changes, as @clementval pointed out. I'd suggest a discourse post or guideline patch so we can have the discussion if this is really helpful, especially as an automatically applied code change.

I don't think all cleanups need style guide wording (see a lot of @kazu's other cleanups, such as migrating to range-based algorithms and other LLVM wrappers rather than iterator-pair based standard algorithms). But yeah, totally open to some discussion about this particular one.

There is two problems with this approach (no style guide wording and no "announcement"):

1. People might have reasonable arguments against and no chance to bring them up.
2. People do not realize what happened and will continue to introduce the "old" style which somewhat defeats the purpose. Playing devils advocate, people might just undo the style changes with the same argument they were introduced (no need for discussion or style guide changes).

Not sure if it needs to go on discourse, or we can have it here - I'd be OK with a more conservative cleanup of mapping "hasValue" to "has_value" and existing implicit bool checking remains the same. (& getValue -> value() and op* remains the same.

I'm totally fine with hasValue -> has_value (and similar) to phase out llvm::Optional (if that is the actual plan). We can also "encourage people" to use these by deprecating the old ones.

Though I personally find bool+* to be generally easier, because it's consistent with other things like pointers, so I don't have to think about which syntax to use for this optional-ish thing. (admittedly optional<bool> is particularly hairy) - but it's not worth a big discussion/trying to make that policy change, I don't think.

It's not only optional<bool>, int, ptr, can also be confusing.
The example I flagged in on of the patches changed:

`if (... && Size1.hasValue() && ...)` to `if (... && Size1 && ...)`.

I am very certain most people will read the former as "Size1 has a value" and the latter as "Size1 is not null". Which is arguably not an improvement.

@kazu - sorry for the poor advice. Would it be especially hard to fixup the already refactored code to preserve the spelling-type used (eg: migrating hasValue -> has_value, etc)? (could probably do this without actually reverting all the cleanup upstream - revert it locally, then apply a modified refactoring that preserves spelling, then squash those commits and you should have a patch that only changes the places that need to be updated to the explicit spelling?)

Would that be sufficient/good @jdoerfert ?

I don't think we should revert everything, that is causing churn. We should read through and flag the few problematic situations though which we move to std::Optional spelling. Also deprecating the old spelling methods seems reasonable, or just adding a comment.

[dblaikie]

@kazu - sorry for the poor advice. Would it be especially hard to fixup the already refactored code to preserve the spelling-type used (eg: migrating hasValue -> has_value, etc)? (could probably do this without actually reverting all the cleanup upstream - revert it locally, then apply a modified refactoring that preserves spelling, then squash those commits and you should have a patch that only changes the places that need to be updated to the explicit spelling?)

Would that be sufficient/good @jdoerfert ?

I don't think we should revert everything, that is causing churn. We should read through and flag the few problematic situations though which we move to std::Optional spelling.

sorry, what I was suggesting was a /local/ revert (not upstreamed), then apply the more nuanced transformation (that preserves explicit test/access where it's written originally) on top of that, then merge those two commits and the result should be one commit that touches only the places that need to be corrected that lost the explicit access/testing.

Also deprecating the old spelling methods seems reasonable, or just adding a comment.

I think that probably won't be necessary - I assume @kazu plans to complete this migration and remove the old methods entirely once all the uses are cleaned up. But might be necessary to use deprecation attributes for a while to help out-of-tree folks migrate off.

[kazu]

Thank you for the feedback. I've created a discourse topic:

https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716

[MaskRay]

llvm/include/llvm/ADT/Optional.h

208

std::optional::value has undesired exception checking semantics: https://en.cppreference.com/w/cpp/utility/optional/value
llvm-project defaults to not use exceptions so this is wasteful.

And see _LIBCPP_AVAILABILITY_BAD_OPTIONAL_ACCESS in libcxx/ which has __attribute__((availability(macos,strict,introduced=10.13)))/etc. It seems that Apple contributors no longer test such an old SDK (we haven't heard of complaints about unavailablity of value, but Chrome toolchain folks do target 10.12).

value likely needs a LLVM_DEPRECATED. See https://discourse.llvm.org/t/deprecating-llvm-optional-x-hasvalue-getvalue-getvalueor/63716/19?u=maskray

llvm/include/llvm/ADT/Optional.h

public:
 
   void reset() noexcept {
     if (hasVal) {
       val.~T();
       hasVal = false;
     }
   }
 
+  constexpr bool has_value() const noexcept { return hasVal; }
   constexpr bool hasValue() const noexcept { return hasVal; }
 
+  T &value() &noexcept {
+    assert(hasVal);
+    return val;
+  }
   T &getValue() &noexcept {
     assert(hasVal);
     return val;
   }
+  constexpr T const &value() const &noexcept {
+    assert(hasVal);
+    return val;
+  }
   constexpr T const &getValue() const &noexcept {
     assert(hasVal);
     return val;
   }
+  T &&value() &&noexcept {
+    assert(hasVal);
+    return std::move(val);
+  }
   T &&getValue() &&noexcept {
     assert(hasVal);
     return std::move(val);
   }
 
   template <class... Args> void emplace(Args &&...args) {
     reset();
     ::new ((void *)std::addressof(val)) T(std::forward<Args>(args)...);
public:
 
   void reset() noexcept {
     if (hasVal) {
       val.~T();
       hasVal = false;
     }
   }
 
+  constexpr bool has_value() const noexcept { return hasVal; }
   constexpr bool hasValue() const noexcept { return hasVal; }
 
+  T &value() &noexcept {
+    assert(hasVal);
+    return val;
+  }
   T &getValue() &noexcept {
     assert(hasVal);
     return val;
   }
+  constexpr T const &value() const &noexcept {
+    assert(hasVal);
+    return val;
+  }
   constexpr T const &getValue() const &noexcept {
     assert(hasVal);
     return val;
   }
+  T &&value() &&noexcept {
+    assert(hasVal);
+    return std::move(val);
+  }
   T &&getValue() &&noexcept {
     assert(hasVal);
     return std::move(val);
   }
 
   template <class... Args> void emplace(Args &&...args) {
     reset();
     ::new ((void *)std::addressof(val)) T(std::forward<Args>(args)...);
Optional &operator=(const T &y) {
     return *this;
   }
   Optional &operator=(const Optional &O) = default;
 
   void reset() { Storage.reset(); }
 
   constexpr const T *getPointer() const { return &Storage.getValue(); }
   T *getPointer() { return &Storage.getValue(); }
+  constexpr const T &value() const & { return Storage.getValue(); }
   constexpr const T &getValue() const & { return Storage.getValue(); }
+  T &value() & { return Storage.getValue(); }
   T &getValue() & { return Storage.getValue(); }
 
   constexpr explicit operator bool() const { return hasValue(); }
+  constexpr bool has_value() const { return Storage.hasValue(); }
   constexpr bool hasValue() const { return Storage.hasValue(); }
   constexpr const T *operator->() const { return getPointer(); }
   T *operator->() { return getPointer(); }
   constexpr const T &operator*() const & { return getValue(); }
   T &operator*() & { return getValue(); }
 
+  template <typename U> constexpr T value_or(U &&value) const & {
+    return hasValue() ? getValue() : std::forward<U>(value);
+  }
   template <typename U> constexpr T getValueOr(U &&value) const & {
     return hasValue() ? getValue() : std::forward<U>(value);
   }
 
   /// Apply a function to the value if present; otherwise return None.
   template <class Function>
   auto map(const Function &F) const & -> Optional<decltype(F(getValue()))> {
     if (*this)
       return F(getValue());
     return None;
   }
 
+  T &&value() && { return std::move(Storage.getValue()); }
   T &&getValue() && { return std::move(Storage.getValue()); }
   T &&operator*() && { return std::move(Storage.getValue()); }
 
+  template <typename U> T value_or(U &&value) && {
+    return hasValue() ? std::move(getValue()) : std::forward<U>(value);
+  }
   template <typename U> T getValueOr(U &&value) && {
     return hasValue() ? std::move(getValue()) : std::forward<U>(value);
   }
 
   /// Apply a function to the value if present; otherwise return None.
   template <class Function>
   auto map(const Function &F)
       && -> Optional<decltype(F(std::move(*this).getValue()))> {

llvm/unittests/ADT/OptionalTest.cpp

static_assert(std::is_trivially_copyable<Optional<int>>::value,
               "trivially copyable");
 
 static_assert(std::is_trivially_copyable<Optional<std::array<int, 3>>>::value,
               "trivially copyable");
 
 void OptionalWorksInConstexpr() {
   constexpr auto x1 = Optional<int>();
   constexpr Optional<int> x2{};
+  static_assert(!x1.has_value() && !x2.has_value(),
+                "Default construction and hasValue() are contexpr");
   static_assert(!x1.hasValue() && !x2.hasValue(),
                 "Default construction and hasValue() are contexpr");
   constexpr auto y1 = Optional<int>(3);
   constexpr Optional<int> y2{3};
+  static_assert(y1.value() == y2.value() && y1.value() == 3,
+                "Construction with value and getValue() are constexpr");
   static_assert(y1.getValue() == y2.getValue() && y1.getValue() == 3,
                 "Construction with value and getValue() are constexpr");
   static_assert(Optional<int>{3} >= 2 && Optional<int>{1} < Optional<int>{2},
                 "Comparisons work in constexpr");
 }
 
 namespace {
 
TEST(OptionalTest, InPlaceConstructionNonDefaultConstructibleTest) {
   { Optional<NonDefaultConstructible> A{in_place, 1}; }
   EXPECT_EQ(0u, NonDefaultConstructible::CopyConstructions);
   EXPECT_EQ(0u, NonDefaultConstructible::CopyAssignments);
   EXPECT_EQ(1u, NonDefaultConstructible::Destructions);
 }
 
 TEST(OptionalTest, GetValueOr) {
   Optional<int> A;
+  EXPECT_EQ(42, A.value_or(42));
   EXPECT_EQ(42, A.getValueOr(42));
 
   A = 5;
+  EXPECT_EQ(5, A.value_or(42));
   EXPECT_EQ(5, A.getValueOr(42));
 }
 
 struct MultiArgConstructor {
   int x, y;
   MultiArgConstructor(int x, int y) : x(x), y(y) {}
   explicit MultiArgConstructor(int x, bool positive)
     : x(x), y(positive ? x : -x) {}

 static_assert(!std::is_trivially_copyable<Optional<MultiArgConstructor>>::value,
               "not trivially copyable");
 
 TEST(OptionalTest, Emplace) {
   MultiArgConstructor::ResetCounts();
   Optional<MultiArgConstructor> A;
   
   A.emplace(1, 2);
+  EXPECT_TRUE(A.has_value());
   EXPECT_TRUE(A.hasValue());
   EXPECT_EQ(1, A->x);
   EXPECT_EQ(2, A->y);
   EXPECT_EQ(0u, MultiArgConstructor::Destructions);
 
   A.emplace(5, false);
+  EXPECT_TRUE(A.has_value());
   EXPECT_TRUE(A.hasValue());
   EXPECT_EQ(5, A->x);
   EXPECT_EQ(-5, A->y);
   EXPECT_EQ(1u, MultiArgConstructor::Destructions);
 }
 
 TEST(OptionalTest, InPlaceConstructionMultiArgConstructorTest) {
   MultiArgConstructor::ResetCounts();
   {
     Optional<MultiArgConstructor> A{in_place, 1, 2};
+    EXPECT_TRUE(A.has_value());
     EXPECT_TRUE(A.hasValue());
     EXPECT_EQ(1, A->x);
     EXPECT_EQ(2, A->y);
     Optional<MultiArgConstructor> B{in_place, 5, false};
+    EXPECT_TRUE(B.has_value());
     EXPECT_TRUE(B.hasValue());
     EXPECT_EQ(5, B->x);
     EXPECT_EQ(-5, B->y);
     EXPECT_EQ(0u, MultiArgConstructor::Destructions);
   }
   EXPECT_EQ(2u, MultiArgConstructor::Destructions);
 }
 

 TEST(OptionalTest, DeletedCopyStringMap) {
   // Old versions of gcc (7.3 and prior) instantiate the copy constructor when
   // std::is_trivially_copyable is instantiated.  This test will fail
   // compilation if std::is_trivially_copyable is used in the OptionalStorage
   // specialization condition by gcc <= 7.3.
   Optional<NoCopyStringMap> TestInstantiation;
 }
 
+TEST(OptionalTest, MoveValueOr) {
+  Optional<MoveOnly> A;
+
+  MoveOnly::ResetCounts();
+  EXPECT_EQ(42, std::move(A).value_or(MoveOnly(42)).val);
+  EXPECT_EQ(1u, MoveOnly::MoveConstructions);
+  EXPECT_EQ(0u, MoveOnly::MoveAssignments);
+  EXPECT_EQ(2u, MoveOnly::Destructions);
+
+  A = MoveOnly(5);
+  MoveOnly::ResetCounts();
+  EXPECT_EQ(5, std::move(A).value_or(MoveOnly(42)).val);
+  EXPECT_EQ(1u, MoveOnly::MoveConstructions);
+  EXPECT_EQ(0u, MoveOnly::MoveAssignments);
+  EXPECT_EQ(2u, MoveOnly::Destructions);
+}
+
 TEST(OptionalTest, MoveGetValueOr) {
   Optional<MoveOnly> A;
 
   MoveOnly::ResetCounts();
   EXPECT_EQ(42, std::move(A).getValueOr(MoveOnly(42)).val);
   EXPECT_EQ(1u, MoveOnly::MoveConstructions);
   EXPECT_EQ(0u, MoveOnly::MoveAssignments);
   EXPECT_EQ(2u, MoveOnly::Destructions);