[ADT] Add getAsOr to Optional

[njames93]

Summary

Adds a method getAsOr to Optional, This functions somewhat like getValueOr however it casts the value to a different type.
This is particularily useful if the type contained in the optional is expensive to construct.

Typical use cases would be for Optional<std::string>, It would often only be needed to access the value as a StringRef.
Similar approach can be made for ArrayRef.

This function is enabled for any type that can be statically casted from the type contained in the Optional

[dblaikie]

I forget: Have we already decided that Optional won't be replaced by std::optional? If not, then would be good to not add API surface area that doesn't match std::optional (instead preferring non-member functions, for instance).

If that's already been decided in some previous reviews, etc, this sounds OK I guess. Probably doesn't need the enable_if, I'd have thought (could just let the implementation fail as usual - there aren't competing member functions, etc) & the parameter should be U&& to use with std::forward, avoid unnecessary copies, etc.

[njames93]

Remove call to forward as it serves no purpose for the use case

[njames93]

In D85072#2189300, @dblaikie wrote:

I forget: Have we already decided that Optional won't be replaced by std::optional? If not, then would be good to not add API surface area that doesn't match std::optional (instead preferring non-member functions, for instance).
If that's already been decided in some previous reviews, etc, this sounds OK I guess.

If that's the case then I'd agree with that.
But I kind of gathered it wouldn't be the case considering how much the llvm::Optional API already differs from std::optional.

Probably doesn't need the enable_if, I'd have thought (could just let the implementation fail as usual - there aren't competing member functions, etc)

Fair enough point there.

the parameter should be U&& to use with std::forward, avoid unnecessary copies, etc.

I'm inclined to disagree with that point, this is mean't to be used with types that are cheap to construct and usually passed by value.
Probably don't need the call to std::forward though.

[njames93]

Delete the rvalue reference version as it can create dangling references

[dblaikie]

In D85072#2190302, @njames93 wrote:

In D85072#2189300, @dblaikie wrote:

I forget: Have we already decided that Optional won't be replaced by std::optional? If not, then would be good to not add API surface area that doesn't match std::optional (instead preferring non-member functions, for instance).
If that's already been decided in some previous reviews, etc, this sounds OK I guess.

If that's the case then I'd agree with that.
But I kind of gathered it wouldn't be the case considering how much the llvm::Optional API already differs from std::optional.

Hmm, I'm not so sure - getValueOr, for instance, is in std::optional. (value_or)

You're right that 'map', for instance, is not in std::optional, though. That and maybe the "create" function are the non-std::optional API?

I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.

llvm/include/llvm/ADT/Optional.h

295–297

I have reservations about this - for the same reason that StringRef, for instance, doesn't block being built from an rvalue (because you might (& in StringRef's case, do quite often) create one from a temporary only for use within a single statement).

Matching the sort of overload/semantics of getValueOr seems appropriate here.

Is there a particular example you have in mind where this would be a problem?

[njames93]

In D85072#2191557, @dblaikie wrote:>

Hmm, I'm not so sure - getValueOr, for instance, is in std::optional. (value_or)

You're right that 'map', for instance, is not in std::optional, though. That and maybe the "create" function are the non-std::optional API?

And getPointer

I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.

non-member could work I guess, and could be adapted for other cases, std::optional or pointers spring to mind.

llvm/include/llvm/ADT/Optional.h

295–297

Fair enough, I'll remove this. Though is it wise to use static_cast<U>(getValue()) or static_cast<U>(std::move(getValue()))

[dblaikie]

In D85072#2193251, @njames93 wrote:

In D85072#2191557, @dblaikie wrote:>

Hmm, I'm not so sure - getValueOr, for instance, is in std::optional. (value_or)

You're right that 'map', for instance, is not in std::optional, though. That and maybe the "create" function are the non-std::optional API?

And getPointer

Ah, right you are - I was thinking std::optional had "get()" like std::unique_ptr, but I must've got them jumbled up.

I think if this new function was a non-member, it'd probably tidier - wouldn't need an rvalue and non-rvalue overload, as it could use std::forward on both arguments.

non-member could work I guess, and could be adapted for other cases, std::optional or pointers spring to mind.

Yep. (also, wouldn't mind if the test was a bit more narrow - if possible, not using non-trivial standard library types - using explicitly written types (not necessarily written just for the test - there are other common utility/stub/mock types in this file and other test files that can be generalized a bit and reused, etc) to test certain features - narrows the scope a bit so the test isn't dependent on too much else)

llvm/include/llvm/ADT/Optional.h

public:
   const T &operator*() const LLVM_LVALUE_FUNCTION { return getValue(); }
   T &operator*() LLVM_LVALUE_FUNCTION { return getValue(); }
 
   template <typename U>
   constexpr T getValueOr(U &&value) const LLVM_LVALUE_FUNCTION {
     return hasValue() ? getValue() : std::forward<U>(value);
   }
 
+  /// Returns the value contained in this as a ``U`` if present, otherwise
+  /// return \p Default.
+  template <typename U> U getAsOr(U Default) const LLVM_LVALUE_FUNCTION {
+    if (hasValue())
+      return static_cast<U>(getValue());
+    return Default;
+  }
+
   /// Apply a function to the value if present; otherwise return None.
   template <class Function>
   auto map(const Function &F) const LLVM_LVALUE_FUNCTION
       -> Optional<decltype(F(getValue()))> {
     if (*this) return F(getValue());
     return None;
   }
 
 #if LLVM_HAS_RVALUE_REFERENCE_THIS
   T &&getValue() && { return std::move(Storage.getValue()); }
   T &&operator*() && { return std::move(Storage.getValue()); }
 
   template <typename U>
   T getValueOr(U &&value) && {
     return hasValue() ? std::move(getValue()) : std::forward<U>(value);
   }
 
+  // Delete this as it can lead to dangling reference issues if ``U`` references
+  // the value held in the Optional.
+  template <typename U> U getAsOr(U Default) && = delete;
+
   /// 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()))> {
     if (*this) return F(std::move(*this).getValue());
     return None;
   }
 #endif

llvm/unittests/ADT/OptionalTest.cpp

 //===- llvm/unittest/ADT/OptionalTest.cpp - Optional unit tests -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/Optional.h"
+#include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Support/raw_ostream.h"
 #include "gtest/gtest-spi.h"
 #include "gtest/gtest.h"
 
 #include <array>
-
+#include <initializer_list>
 
 using namespace llvm;
 
 static_assert(is_trivially_copyable<Optional<int>>::value,
           "trivially copyable");
 
 static_assert(is_trivially_copyable<Optional<std::array<int, 3>>>::value,
               "trivially copyable");

 TEST_F(OptionalTest, GetValueOr) {
   Optional<int> A;
   EXPECT_EQ(42, A.getValueOr(42));
 
   A = 5;
   EXPECT_EQ(5, A.getValueOr(42));
 }
 
+TEST_F(OptionalTest, GetAsOr) {
+  Optional<std::string> String;
+  StringRef Default{"DefaultOverflowSmallStringOpt"};
+  StringRef Value{"Value"};
+  EXPECT_EQ(Default, String.getAsOr(Default));
+  String.emplace(Value);
+  EXPECT_EQ(Value, String.getAsOr(Default));
+
+  int Item = 0;
+  ArrayRef<int> OneItem = makeArrayRef(Item);
+  ArrayRef<int> Empty;
+
+  Optional<llvm::SmallVector<int, 4>> SmallVec;
+  EXPECT_EQ(SmallVec.getAsOr(Empty).size(), 0U);
+  EXPECT_EQ(SmallVec.getAsOr(ArrayRef<int>{}).size(), 0U);
+  EXPECT_EQ(SmallVec.getAsOr(OneItem).size(), 1U);
+  SmallVec.emplace(std::initializer_list<int>{0, 1, 2, 3});
+  EXPECT_EQ(SmallVec.getAsOr(Empty).size(), 4U);
+
+  Optional<std::vector<int>> Vector;
+  EXPECT_EQ(Vector.getAsOr(Empty).size(), 0U);
+  EXPECT_EQ(Vector.getAsOr(OneItem).size(), 1U);
+  Vector.emplace(std::initializer_list<int>{0, 1, 2, 3});
+  EXPECT_EQ(Vector.getAsOr(Empty).size(), 4U);
+}
+
 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) {}
 
   MultiArgConstructor(const MultiArgConstructor &) = delete;
   MultiArgConstructor(MultiArgConstructor &&) = delete;