[libc-commits] [libc] [libc] Move in_use into OptionalStorage (PR #73569)

[via libc-commits]

https://github.com/michaelrj-google created https://github.com/llvm/llvm-project/pull/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.


>From 5a64e946c94b0eb63e32f736a193805f46f47ccb Mon Sep 17 00:00:00 2001
From: Michael Jones <michaelrj at google.com>
Date: Mon, 27 Nov 2023 13:12:56 -0800
Subject: [PATCH] [libc] Move in_use into OptionalStorage

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.
---
 libc/src/__support/CPP/optional.h             | 47 ++++++++++++-------
 libc/test/src/__support/CPP/optional_test.cpp | 20 ++++----
 2 files changed, 43 insertions(+), 24 deletions(-)

diff --git a/libc/src/__support/CPP/optional.h b/libc/src/__support/CPP/optional.h
index 02e8395b871ba29..2c0f639a2b342cc 100644
--- a/libc/src/__support/CPP/optional.h
+++ b/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,11 +105,7 @@ 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;
@@ -100,8 +113,10 @@ template <typename T> class optional {
 
   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;
   }
diff --git a/libc/test/src/__support/CPP/optional_test.cpp b/libc/test/src/__support/CPP/optional_test.cpp
index f9254d42c9a9ea8..b2c8545eb36f078 100644
--- a/libc/test/src/__support/CPP/optional_test.cpp
+++ b/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