From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Sat, 30 Jul 2022 01:12:17 +0000 (+0000)
Subject: Support non-trivial Optional values (#1106)
X-Git-Tag: SQUID_6_0_1~140
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=76ed29975b545600970d026c49b016e8fe952ab0;p=thirdparty%2Fsquid.git

Support non-trivial Optional values (#1106)

Commit 7224761 skipped this std::optional feature when adding initial
Optional implementation because that feature is difficult to support.
However, several in-progress projects now need to make non-trivial
objects Optional. This implementation maintains key Optional invariants,
matching those of std::optional:

* A valueless Optional object does not construct or destruct the value.
* A valued Optional object supports move/copy ops supported by value.

Maintaining the first invariant is tricky:

* Union prevents value construction/destruction in a valueless Optional.
* Explicit destructor call destructs the value in a _valued_ Optional.
* A dummy union member works around a C++ requirement for constexpr
  unions to have at least one active (i.e. initialized) member. Since a
  valueless Optional cannot initialize the value, we need another union
  member that we can initialize.
* We use an _anonymous_ union because C++ places more requirements on
  named unions, triggering a more complex implementation with
  placement-new and to-be-deprecated std::aligned_storage_t tricks!

XXX: This simplified implementation violates the following std::optional
invariant. We believe that this violation does not hurt the current and
foreseeable Squid code. In our tests, optimizing compilers still
optimize Optional<Trivial> destructors away.

* std::optional<Value> is trivially destructible if Value is.
---

diff --git a/src/base/Optional.h b/src/base/Optional.h
index 9d59923cb3..72440e8e10 100644
--- a/src/base/Optional.h
+++ b/src/base/Optional.h
@@ -28,14 +28,56 @@ template <typename Value>
 class Optional
 {
 public:
-    // std::optional supports non-trivial types as well, but we
-    // do not want to fiddle with unions to disable default Value constructor
-    // until that work becomes necessary
-    static_assert(std::is_trivial<Value>::value, "Value is trivial");
-
-    constexpr Optional() noexcept {}
+    constexpr Optional() noexcept: dummy_() {}
     constexpr explicit Optional(const Value &v): value_(v), hasValue_(true) {}
 
+    ~Optional()
+    {
+        // XXX: This simplified implementation does not keep the destructor
+        // trivial for trivial Value types, but optimizing compilers still
+        // optimize such destruction away, and that is sufficient for our
+        // current needs.
+        reset();
+    }
+
+    Optional(const Optional &other): Optional()
+    {
+        if (other.hasValue_)
+            *this = other.value_;
+    }
+
+    Optional &operator =(const Optional &other)
+    {
+        if (this != &other) {
+            if (other.hasValue_)
+                *this = other.value_;
+            else
+                reset();
+        }
+        return *this;
+    }
+
+    Optional(Optional<Value> &&other): Optional()
+    {
+        if (other.hasValue_) {
+            *this = std::move(other.value_);
+            // no other.reset() per std::optional move semantics
+        }
+    }
+
+    Optional &operator =(Optional<Value> &&other)
+    {
+        if (this != &other) {
+            if (other.hasValue_) {
+                *this = std::move(other.value_);
+                // no other.reset() per std::optional move semantics
+            } else {
+                reset();
+            }
+        }
+        return *this;
+    }
+
     constexpr explicit operator bool() const noexcept { return hasValue_; }
     constexpr bool has_value() const noexcept { return hasValue_; }
 
@@ -60,8 +102,22 @@ public:
         return *this;
     }
 
+    void reset() {
+        if (hasValue_) {
+            hasValue_ = false;
+            value_.~Value();
+        }
+    }
+
 private:
-    Value value_; // stored value; inaccessible/uninitialized unless hasValue_
+    union {
+        /// unused member that helps satisfy various C++ union requirements
+        struct {} dummy_;
+
+        /// stored value; inaccessible/uninitialized unless hasValue_
+        Value value_;
+    };
+
     bool hasValue_ = false;
 };