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

[via libc-commits]

Author: michaelrj-google
Date: 2023-11-27T13:31:10-08:00
New Revision: f90f036efbe6879ecf1c9ff8cf0fb956b5ceb877

URL: https://github.com/llvm/llvm-project/commit/f90f036efbe6879ecf1c9ff8cf0fb956b5ceb877
DIFF: https://github.com/llvm/llvm-project/commit/f90f036efbe6879ecf1c9ff8cf0fb956b5ceb877.diff

LOG:     [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.

Added: 
    

Modified: 
    libc/src/__support/CPP/optional.h
    libc/test/src/__support/CPP/optional_test.cpp

Removed: 
    


################################################################################
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 
diff erence 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 
diff erence 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