[libc] Move in_use into OptionalStorage (#73569)

The previous optional class would call the destructor on a non-trivially
destructible object regardless of if it had already been reset. This
patch fixes this by moving tracking for if the object exists into the
internal storage class for optional.

Author: michaelrj-google

libc/src/__support/CPP/optional.h

@@ -25,7 +25,7 @@ struct nullopt_t {
 LIBC_INLINE_VAR constexpr nullopt_t nullopt{};
 
 // This is very simple implementation of the std::optional class. It makes
-// several assumptions that the underlying type is trivially constructable,
+// several assumptions that the underlying type is trivially constructible,
 // copyable, or movable.
 template <typename T> class optional {
   template <typename U, bool = !is_trivially_destructible<U>::value>
@@ -35,46 +35,63 @@ template <typename T> class optional {
       U stored_value;
     };
 
-    LIBC_INLINE ~OptionalStorage() { stored_value.~U(); }
+    bool in_use = false;
+
+    LIBC_INLINE ~OptionalStorage() { reset(); }
+
     LIBC_INLINE constexpr OptionalStorage() : empty() {}
 
     template <typename... Args>
     LIBC_INLINE constexpr explicit OptionalStorage(in_place_t, Args &&...args)
         : stored_value(forward<Args>(args)...) {}
+
+    LIBC_INLINE constexpr void reset() {
+      if (in_use)
+        stored_value.~U();
+      in_use = false;
+    }
   };
 
+  // The only difference is that this type U doesn't have a nontrivial
+  // destructor.
   template <typename U> struct OptionalStorage<U, false> {
     union {
       char empty;
       U stored_value;
     };
 
-    // The only difference is that this class doesn't have a destructor.
+    bool in_use = false;
+
     LIBC_INLINE constexpr OptionalStorage() : empty() {}
 
     template <typename... Args>
     LIBC_INLINE constexpr explicit OptionalStorage(in_place_t, Args &&...args)
         : stored_value(forward<Args>(args)...) {}
+
+    LIBC_INLINE constexpr void reset() { in_use = false; }
   };
 
   OptionalStorage<T> storage;
-  bool in_use = false;
 
 public:
   LIBC_INLINE constexpr optional() = default;
   LIBC_INLINE constexpr optional(nullopt_t) {}
 
-  LIBC_INLINE constexpr optional(const T &t)
-      : storage(in_place, t), in_use(true) {}
+  LIBC_INLINE constexpr optional(const T &t) : storage(in_place, t) {
+    storage.in_use = true;
+  }
   LIBC_INLINE constexpr optional(const optional &) = default;
 
-  LIBC_INLINE constexpr optional(T &&t)
-      : storage(in_place, move(t)), in_use(true) {}
+  LIBC_INLINE constexpr optional(T &&t) : storage(in_place, move(t)) {
+    storage.in_use = true;
+  }
   LIBC_INLINE constexpr optional(optional &&O) = default;
 
   template <typename... ArgTypes>
   LIBC_INLINE constexpr optional(in_place_t, ArgTypes &&...Args)
-      : storage(in_place, forward<ArgTypes>(Args)...), in_use(true) {}
+      : storage(in_place, forward<ArgTypes>(Args)...) {
+    storage.in_use = true;
+  }
 
   LIBC_INLINE constexpr optional &operator=(T &&t) {
     storage = move(t);
@@ -88,20 +105,18 @@ template <typename T> class optional {
   }
   LIBC_INLINE constexpr optional &operator=(const optional &) = default;
 
-  LIBC_INLINE constexpr void reset() {
-    if (in_use)
-      storage.~OptionalStorage();
-    in_use = false;
-  }
+  LIBC_INLINE constexpr void reset() { storage.reset(); }
 
   LIBC_INLINE constexpr const T &value() const & {
     return storage.stored_value;
   }
 
   LIBC_INLINE constexpr T &value() & { return storage.stored_value; }
 
-  LIBC_INLINE constexpr explicit operator bool() const { return in_use; }
-  LIBC_INLINE constexpr bool has_value() const { return in_use; }
+  LIBC_INLINE constexpr explicit operator bool() const {
+    return storage.in_use;
+  }
+  LIBC_INLINE constexpr bool has_value() const { return storage.in_use; }
   LIBC_INLINE constexpr const T *operator->() const {
     return &storage.stored_value;
   }

libc/test/src/__support/CPP/optional_test.cpp

@@ -42,14 +42,18 @@ TEST(LlvmLibcOptionalTest, Tests) {
 
   // For this test case, the destructor increments the pointed-to value.
   int holding = 1;
-  optional<Contrived> Complicated(&holding);
-  // Destructor was run once as part of copying the object.
-  ASSERT_EQ(holding, 2);
-  // Destructor was run a second time as part of destruction.
-  Complicated.reset();
-  ASSERT_EQ(holding, 3);
-  // Destructor was not run a third time as the object is already destroyed.
-  Complicated.reset();
+  {
+    optional<Contrived> Complicated(&holding);
+    // Destructor was run once as part of copying the object.
+    ASSERT_EQ(holding, 2);
+    // Destructor was run a second time as part of destruction.
+    Complicated.reset();
+    ASSERT_EQ(holding, 3);
+    // Destructor was not run a third time as the object is already destroyed.
+    Complicated.reset();
+    ASSERT_EQ(holding, 3);
+  }
+  // Make sure the destructor isn't called when the optional is destroyed.
   ASSERT_EQ(holding, 3);
 
   // Test that assigning an optional to another works when set