[libc-commits] [PATCH] D150211: [libc] Extend optional to support non trivially destructible types

[Michael Jones via Phabricator via libc-commits]

michaelrj added a comment.

I have a bunch of comments, but I think I'm willing to accept the update to `optional` once review is done. The important part is making it more clear that this patch is focused on modifying `OptionalStorage` to support destructors.



================
Comment at: libc/src/__support/CPP/optional.h:36-61
+  template <typename U, bool = is_trivially_destructible<U>::value>
+  struct OptionalStorage {
     union {
       char empty;
       U stored_value;
     };
 
----------------
I think having the internal `OptionalStorage` struct inside `OptionalStorageBase` is confusing. Specifically it makes it unclear which functions are part of `OptionalStorageBase` vs `OptionalStorage` in a code-review setting where things like colored parentheses aren't available.


================
Comment at: libc/src/__support/CPP/optional.h:37
+  template <typename U, bool = is_trivially_destructible<U>::value>
+  struct OptionalStorage {
     union {
----------------
This struct shouldn't be named the same as the other, completely separate struct that inherits from `OptionalStorageBase`. It's very confusing to read.


================
Comment at: libc/src/__support/CPP/optional.h:67
 
-    LIBC_INLINE U &value() & { return stored_value; }
-    LIBC_INLINE constexpr U const &value() const & { return stored_value; }
-    LIBC_INLINE U &&value() && { return move(stored_value); }
-  };
+public:
+  LIBC_INLINE ~OptionalStorageBase() = default;
----------------
you don't need to specify public here, `struct`s are public by default.


================
Comment at: libc/src/__support/CPP/optional.h:104-110
+  LIBC_INLINE constexpr OptionalStorage() = default;
+  LIBC_INLINE constexpr OptionalStorage(const OptionalStorage &) = default;
+  LIBC_INLINE constexpr OptionalStorage(OptionalStorage &&) = default;
+  LIBC_INLINE constexpr OptionalStorage &
+  operator=(const OptionalStorage &) = default;
+  LIBC_INLINE constexpr OptionalStorage &
+  operator=(OptionalStorage &&) = default;
----------------
if these are all default, you don't need to specify them.


================
Comment at: libc/src/__support/CPP/optional.h:126-132
+  // SFINAE helpers
+  template <typename T2>
+  using __not_self = __not_<is_same<optional, remove_cvref_t<T2>>>;
+  template <typename T2>
+  using __not_tag = __not_<is_same<in_place_t, remove_cvref_t<T2>>>;
+  template <typename... _Cond>
+  using _Requires = enable_if_t<__and_<_Cond...>::value, bool>;
----------------
is this actually necessary? The objects that we're wrapping in optional are mostly simple structs or primitives.


================
Comment at: libc/src/__support/CPP/type_traits.h:63
 
-template <typename T> struct add_rvalue_reference : type_identity<T &&> {};
-
----------------
why is this removed?


================
Comment at: libc/src/__support/CPP/type_traits.h:198
 
+template <typename...> struct __and_;
+
----------------
this is unnecessary. You can just use `&&` and `enable_if_t`. See `span.h` for an example of how that works. Same for `__not_`


================
Comment at: libc/src/__support/UInt.h:379
     if (other == 0) {
-      return nullopt;
+      return {};
     }
----------------
this doesn't need to be changed


================
Comment at: libc/test/src/__support/CPP/optional_test.cpp:46
   optional<Contrived> Complicated(&holding);
-  // Destructor was run once as part of copying the object.
-  ASSERT_EQ(holding, 2);
----------------
is the object no longer copied? If so, is that intended behavior?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150211