[ADT] Make Optional a literal type.

[varungandhi-apple]

Summary

This allows returning Optional values from constexpr contexts.

[fhahn]

This looks reasonable to me, but it might be good to wait a bit so other people can also chime in.

[rjmccall]

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr? (Note that it's okay for a templated constexpr function to instantiate to something that does stuff that's not constexpr, so this wouldn't be a major new restriction on the constructor.)

[dblaikie]

llvm/unittests/ADT/OptionalTest.cpp

20–23

This might be a bit unnecessary, if the test below is adequate? (this seems like a place where testing on the public interface would be sufficient & make it easier if the implementation details are refactored later)

24

is_literal_type is deprecated in C++17 and removed in C++20 - might be better to test the property in practical terms by making some constexpr variables (in test cases, or not)

constexpr Optional<int> o;

etc?

[varungandhi-apple]

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr?

The reason I implemented the minimal thing was that I wasn't sure how far I should go. In a Swift PR, @davezarzycki raised the point about constexpr potentially leading to longer compile times even when not used for compile-time computation. I asked some folks internally at Apple and the response I got was along the lines of, "That's surprising... If that's actually the case, it should be reported as a Clang bug". I haven't been able to dedicate time to benchmarking the impact of constexpr on compile time... Since Optional is a currency type, I wasn't sure if people would object to more methods being marked constexpr. In principle, I don't see any reason for any method on Optional to not be constexpr.

[rjmccall]

It's an abstractly reasonable change. I wouldn't worry about some sort of conjectured impact on build times in the absence of specific evidence.

[dblaikie]

In D86354#2240001, @varungandhi-apple wrote:

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr?

The reason I implemented the minimal thing was that I wasn't sure how far I should go. In a Swift PR, @davezarzycki raised the point about constexpr potentially leading to longer compile times even when not used for compile-time computation. I asked some folks internally at Apple and the response I got was along the lines of, "That's surprising... If that's actually the case, it should be reported as a Clang bug". I haven't been able to dedicate time to benchmarking the impact of constexpr on compile time... Since Optional is a currency type, I wasn't sure if people would object to more methods being marked constexpr. In principle, I don't see any reason for any method on Optional to not be constexpr.

That upstream thread doesn't look like it's talking about the impact of constexpr in non-constexpr contexts. Yes, making a variable constexpr when you don't need it to be can have negative compile time consequences - but in Clang I don't believe (again, can be bugs, but it doesn't sound like that swift thread is suggesting the existence of such bugs) a constexpr function called in a non-constexpr context is handled any differently than if it were a non-constexpr function. (GCC is different in this regard and does try to do constexpr things in non-constexpr contexts, though unclear how hard it tries before bailing out to the non-constexpr handling)

[varungandhi-apple]

1. Allow many more operations in constexpr.
2. Update test to reflect use in constexpr instead of relying on implementation details.

[varungandhi-apple]

This comment was removed by varungandhi-apple.

[rjmccall]

I wasn't really asking for massive constexpr-ification, just on the other constructors, but I have no problem with it given that it's done.

[davezarzycki]

In D86354#2240009, @dblaikie wrote:

In D86354#2240001, @varungandhi-apple wrote:

This seems to only allow empty optionals to be constexpr. Should the non-default constructor also be constexpr?

The reason I implemented the minimal thing was that I wasn't sure how far I should go. In a Swift PR, @davezarzycki raised the point about constexpr potentially leading to longer compile times even when not used for compile-time computation. I asked some folks internally at Apple and the response I got was along the lines of, "That's surprising... If that's actually the case, it should be reported as a Clang bug". I haven't been able to dedicate time to benchmarking the impact of constexpr on compile time... Since Optional is a currency type, I wasn't sure if people would object to more methods being marked constexpr. In principle, I don't see any reason for any method on Optional to not be constexpr.

That upstream thread doesn't look like it's talking about the impact of constexpr in non-constexpr contexts. Yes, making a variable constexpr when you don't need it to be can have negative compile time consequences - but in Clang I don't believe (again, can be bugs, but it doesn't sound like that swift thread is suggesting the existence of such bugs) a constexpr function called in a non-constexpr context is handled any differently than if it were a non-constexpr function. (GCC is different in this regard and does try to do constexpr things in non-constexpr contexts, though unclear how hard it tries before bailing out to the non-constexpr handling)

Actually, I was concerned about constexpr affecting non-constexpr contexts. Perhaps it was the gcc behavior that this former coworker was worried about (and I wish I remember who told me this so I could ask them but alas this feedback was many years ago). I'm happy to learn that clang isn't trying to be clever about evaluating constexpr in non-constexpr contexts.

[varungandhi-apple]

(I'm going to wait till at least Monday to land this, in case anyone has objections or would like to suggest changes to the implementation or tests.)

llvm/include/llvm/ADT/Optional.h

union {
     char empty;
     T value;
   };
   bool hasVal;
 
 public:
   ~OptionalStorage() { reset(); }
 
-  OptionalStorage() noexcept : empty(), hasVal(false) {}
+  constexpr OptionalStorage() noexcept : empty(), hasVal(false) {}
 
-  OptionalStorage(OptionalStorage const &other) : OptionalStorage() {
+  constexpr OptionalStorage(OptionalStorage const &other) : OptionalStorage() {
     if (other.hasValue()) {
       emplace(other.value);
     }
   }
-  OptionalStorage(OptionalStorage &&other) : OptionalStorage() {
+  constexpr OptionalStorage(OptionalStorage &&other) : OptionalStorage() {
     if (other.hasValue()) {
       emplace(std::move(other.value));
     }
   }
 
   template <class... Args>
-  explicit OptionalStorage(in_place_t, Args &&... args)
+  constexpr explicit OptionalStorage(in_place_t, Args &&... args)
       : value(std::forward<Args>(args)...), hasVal(true) {}
 
   void reset() noexcept {
     if (hasVal) {
       value.~T();
       hasVal = false;
     }
   }
 
-  bool hasValue() const noexcept { return hasVal; }
+  constexpr bool hasValue() const noexcept { return hasVal; }
 
   T &getValue() LLVM_LVALUE_FUNCTION noexcept {
     assert(hasVal);
     return value;
   }
-  T const &getValue() const LLVM_LVALUE_FUNCTION noexcept {
+  constexpr T const &getValue() const LLVM_LVALUE_FUNCTION noexcept {
     assert(hasVal);
     return value;
   }
 #if LLVM_HAS_RVALUE_REFERENCE_THIS
   T &&getValue() && noexcept {
     assert(hasVal);
     return std::move(value);
   }
union {
     char empty;
     T value;
   };
   bool hasVal = false;
 
 public:
   ~OptionalStorage() = default;
 
-  OptionalStorage() noexcept : empty{} {}
+  constexpr OptionalStorage() noexcept : empty{} {}
 
-  OptionalStorage(OptionalStorage const &other) = default;
-  OptionalStorage(OptionalStorage &&other) = default;
+  constexpr OptionalStorage(OptionalStorage const &other) = default;
+  constexpr OptionalStorage(OptionalStorage &&other) = default;
 
   OptionalStorage &operator=(OptionalStorage const &other) = default;
   OptionalStorage &operator=(OptionalStorage &&other) = default;
 
   template <class... Args>
-  explicit OptionalStorage(in_place_t, Args &&... args)
+  constexpr explicit OptionalStorage(in_place_t, Args &&... args)
       : value(std::forward<Args>(args)...), hasVal(true) {}
 
   void reset() noexcept {
     if (hasVal) {
       value.~T();
       hasVal = false;
     }
   }
 
-  bool hasValue() const noexcept { return hasVal; }
+  constexpr bool hasValue() const noexcept { return hasVal; }
 
   T &getValue() LLVM_LVALUE_FUNCTION noexcept {
     assert(hasVal);
     return value;
   }
-  T const &getValue() const LLVM_LVALUE_FUNCTION noexcept {
+  constexpr T const &getValue() const LLVM_LVALUE_FUNCTION noexcept {
     assert(hasVal);
     return value;
   }
 #if LLVM_HAS_RVALUE_REFERENCE_THIS
   T &&getValue() && noexcept {
     assert(hasVal);
     return std::move(value);
   }
template <typename T> class Optional {
   optional_detail::OptionalStorage<T> Storage;
 
 public:
   using value_type = T;
 
   constexpr Optional() {}
   constexpr Optional(NoneType) {}
 
-  Optional(const T &y) : Storage(optional_detail::in_place_t{}, y) {}
-  Optional(const Optional &O) = default;
+  constexpr Optional(const T &y) : Storage(optional_detail::in_place_t{}, y) {}
+  constexpr Optional(const Optional &O) = default;
 
-  Optional(T &&y) : Storage(optional_detail::in_place_t{}, std::move(y)) {}
-  Optional(Optional &&O) = default;
+  constexpr Optional(T &&y)
+      : Storage(optional_detail::in_place_t{}, std::move(y)) {}
+  constexpr Optional(Optional &&O) = default;
 
   Optional &operator=(T &&y) {
     Storage = std::move(y);
     return *this;
   }
   Optional &operator=(Optional &&O) = default;
 
   /// Create a new object by constructing it in place with the given arguments.
   template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {
     Storage.emplace(std::forward<ArgTypes>(Args)...);
   }
 
-  static inline Optional create(const T *y) {
+  static constexpr Optional create(const T *y) {
     return y ? Optional(*y) : Optional();
   }
 
   Optional &operator=(const T &y) {
     Storage = y;
     return *this;
   }
   Optional &operator=(const Optional &O) = default;
 
   void reset() { Storage.reset(); }
 
-  const T *getPointer() const { return &Storage.getValue(); }
+  constexpr const T *getPointer() const { return &Storage.getValue(); }
   T *getPointer() { return &Storage.getValue(); }
-  const T &getValue() const LLVM_LVALUE_FUNCTION { return Storage.getValue(); }
+  constexpr const T &getValue() const LLVM_LVALUE_FUNCTION {
+    return Storage.getValue();
+  }
   T &getValue() LLVM_LVALUE_FUNCTION { return Storage.getValue(); }
 
-  explicit operator bool() const { return hasValue(); }
-  bool hasValue() const { return Storage.hasValue(); }
-  const T *operator->() const { return getPointer(); }
+  constexpr explicit operator bool() const { return hasValue(); }
+  constexpr bool hasValue() const { return Storage.hasValue(); }
+  constexpr const T *operator->() const { return getPointer(); }
   T *operator->() { return getPointer(); }
-  const T &operator*() const LLVM_LVALUE_FUNCTION { return getValue(); }
+  constexpr 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);
   }
 
   /// Apply a function to the value if present; otherwise return None.
auto map(const Function &F) &&
       -> Optional<decltype(F(std::move(*this).getValue()))> {
     if (*this) return F(std::move(*this).getValue());
     return None;
   }
 #endif
 };
 
 template <typename T, typename U>
-bool operator==(const Optional<T> &X, const Optional<U> &Y) {
+constexpr bool operator==(const Optional<T> &X, const Optional<U> &Y) {
   if (X && Y)
     return *X == *Y;
   return X.hasValue() == Y.hasValue();
 }
 
 template <typename T, typename U>
-bool operator!=(const Optional<T> &X, const Optional<U> &Y) {
+constexpr bool operator!=(const Optional<T> &X, const Optional<U> &Y) {
   return !(X == Y);
 }
 
 template <typename T, typename U>
-bool operator<(const Optional<T> &X, const Optional<U> &Y) {
+constexpr bool operator<(const Optional<T> &X, const Optional<U> &Y) {
   if (X && Y)
     return *X < *Y;
   return X.hasValue() < Y.hasValue();
 }
 
 template <typename T, typename U>
-bool operator<=(const Optional<T> &X, const Optional<U> &Y) {
+constexpr bool operator<=(const Optional<T> &X, const Optional<U> &Y) {
   return !(Y < X);
 }
 
 template <typename T, typename U>
-bool operator>(const Optional<T> &X, const Optional<U> &Y) {
+constexpr bool operator>(const Optional<T> &X, const Optional<U> &Y) {
   return Y < X;
 }
 
 template <typename T, typename U>
-bool operator>=(const Optional<T> &X, const Optional<U> &Y) {
+constexpr bool operator>=(const Optional<T> &X, const Optional<U> &Y) {
   return !(X < Y);
 }
 
-template<typename T>
-bool operator==(const Optional<T> &X, NoneType) {
+template <typename T>
+constexpr bool operator==(const Optional<T> &X, NoneType) {
   return !X;
 }
 
-template<typename T>
-bool operator==(NoneType, const Optional<T> &X) {
+template <typename T>
+constexpr bool operator==(NoneType, const Optional<T> &X) {
   return X == None;
 }
 
-template<typename T>
-bool operator!=(const Optional<T> &X, NoneType) {
+template <typename T>
+constexpr bool operator!=(const Optional<T> &X, NoneType) {
   return !(X == None);
 }
 
-template<typename T>
-bool operator!=(NoneType, const Optional<T> &X) {
+template <typename T>
+constexpr bool operator!=(NoneType, const Optional<T> &X) {
   return X != None;
 }
 
-template <typename T> bool operator<(const Optional<T> &X, NoneType) {
+template <typename T> constexpr bool operator<(const Optional<T> &X, NoneType) {
   return false;
 }
 
-template <typename T> bool operator<(NoneType, const Optional<T> &X) {
+template <typename T> constexpr bool operator<(NoneType, const Optional<T> &X) {
   return X.hasValue();
 }
 
-template <typename T> bool operator<=(const Optional<T> &X, NoneType) {
+template <typename T>
+constexpr bool operator<=(const Optional<T> &X, NoneType) {
   return !(None < X);
 }
 
-template <typename T> bool operator<=(NoneType, const Optional<T> &X) {
+template <typename T>
+constexpr bool operator<=(NoneType, const Optional<T> &X) {
   return !(X < None);
 }
 
-template <typename T> bool operator>(const Optional<T> &X, NoneType) {
+template <typename T> constexpr bool operator>(const Optional<T> &X, NoneType) {
   return None < X;
 }
 
-template <typename T> bool operator>(NoneType, const Optional<T> &X) {
+template <typename T> constexpr bool operator>(NoneType, const Optional<T> &X) {
   return X < None;
 }
 
-template <typename T> bool operator>=(const Optional<T> &X, NoneType) {
+template <typename T>
+constexpr bool operator>=(const Optional<T> &X, NoneType) {
   return None <= X;
 }
 
-template <typename T> bool operator>=(NoneType, const Optional<T> &X) {
+template <typename T>
+constexpr bool operator>=(NoneType, const Optional<T> &X) {
   return X <= None;
 }
 
-template <typename T> bool operator==(const Optional<T> &X, const T &Y) {
+template <typename T>
+constexpr bool operator==(const Optional<T> &X, const T &Y) {
   return X && *X == Y;
 }
 
-template <typename T> bool operator==(const T &X, const Optional<T> &Y) {
+template <typename T>
+constexpr bool operator==(const T &X, const Optional<T> &Y) {
   return Y && X == *Y;
 }
 
-template <typename T> bool operator!=(const Optional<T> &X, const T &Y) {
+template <typename T>
+constexpr bool operator!=(const Optional<T> &X, const T &Y) {
   return !(X == Y);
 }
 
-template <typename T> bool operator!=(const T &X, const Optional<T> &Y) {
+template <typename T>
+constexpr bool operator!=(const T &X, const Optional<T> &Y) {
   return !(X == Y);
 }
 
-template <typename T> bool operator<(const Optional<T> &X, const T &Y) {
+template <typename T>
+constexpr bool operator<(const Optional<T> &X, const T &Y) {
   return !X || *X < Y;
 }
 
-template <typename T> bool operator<(const T &X, const Optional<T> &Y) {
+template <typename T>
+constexpr bool operator<(const T &X, const Optional<T> &Y) {
   return Y && X < *Y;
 }
 
-template <typename T> bool operator<=(const Optional<T> &X, const T &Y) {
+template <typename T>
+constexpr bool operator<=(const Optional<T> &X, const T &Y) {
   return !(Y < X);
 }
 
-template <typename T> bool operator<=(const T &X, const Optional<T> &Y) {
+template <typename T>
+constexpr bool operator<=(const T &X, const Optional<T> &Y) {
   return !(Y < X);
 }
 
-template <typename T> bool operator>(const Optional<T> &X, const T &Y) {
+template <typename T>
+constexpr bool operator>(const Optional<T> &X, const T &Y) {
   return Y < X;
 }
 
-template <typename T> bool operator>(const T &X, const Optional<T> &Y) {
+template <typename T>
+constexpr bool operator>(const T &X, const Optional<T> &Y) {
   return Y < X;
 }
 
-template <typename T> bool operator>=(const Optional<T> &X, const T &Y) {
+template <typename T>
+constexpr bool operator>=(const Optional<T> &X, const T &Y) {
   return !(X < Y);
 }
 
-template <typename T> bool operator>=(const T &X, const Optional<T> &Y) {
+template <typename T>
+constexpr bool operator>=(const T &X, const Optional<T> &Y) {
   return !(X < Y);
 }
 
 raw_ostream &operator<<(raw_ostream &OS, NoneType);
 
 template <typename T, typename = decltype(std::declval<raw_ostream &>()
                                           << std::declval<const T &>())>
 raw_ostream &operator<<(raw_ostream &OS, const Optional<T> &O) {

llvm/unittests/ADT/OptionalTest.cpp

 #include "gtest/gtest-spi.h"
 #include "gtest/gtest.h"
 
 #include <array>
 
 
 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");
 
+void OptionalWorksInConstexpr() {
+  constexpr auto x1 = Optional<int>();
+  constexpr Optional<int> x2{};
+  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.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 {
 
 struct NonDefaultConstructible {
   static unsigned CopyConstructions;
   static unsigned Destructions;
   static unsigned CopyAssignments;
   explicit NonDefaultConstructible(int) {
   }