From 6b4d7b6c19e81c4ee6b635ca4d2f145f38e96d01 Mon Sep 17 00:00:00 2001
From: Ali Mohammad Pur <ali.mpfard@gmail.com>
Date: Sat, 22 May 2021 09:46:40 +0430
Subject: [PATCH] AK: Fix Variant construction from lvalue references

Fixes #7371 and appends its test cases.
---
 AK/Variant.h             | 44 +++++++++++++++++++++-------------------
 Tests/AK/TestVariant.cpp | 41 +++++++++++++++++++++++++++++++++++++
 2 files changed, 64 insertions(+), 21 deletions(-)

diff --git a/AK/Variant.h b/AK/Variant.h
index d558709f11e..34751437931 100644
--- a/AK/Variant.h
+++ b/AK/Variant.h
@@ -106,25 +106,18 @@ template<typename T, typename Base>
 struct VariantConstructors {
     VariantConstructors(T&& t)
     {
-        internal_cast().template set<T>(forward<T>(t), VariantNoClearTag {});
+        internal_cast().clear_without_destruction();
+        internal_cast().set(move(t), VariantNoClearTag {});
+    }
+
+    VariantConstructors(const T& t)
+    {
+        internal_cast().clear_without_destruction();
+        internal_cast().set(t, VariantNoClearTag {});
     }
 
     VariantConstructors() { }
 
-    Base& operator=(const T& value)
-    {
-        Base variant { value };
-        internal_cast() = move(variant);
-        return internal_cast();
-    }
-
-    Base& operator=(T&& value)
-    {
-        Base variant { move(value) };
-        internal_cast() = move(variant);
-        return internal_cast();
-    }
-
 private:
     [[nodiscard]] Base& internal_cast()
     {
@@ -213,6 +206,7 @@ public:
 
     Variant(const Variant& old)
         : Detail::MergeAndDeduplicatePacks<Detail::VariantConstructors<Ts, Variant<Ts...>>...>()
+        , m_data {}
         , m_index(old.m_index)
     {
         Helper::copy_(old.m_index, old.m_data, m_data);
@@ -224,6 +218,7 @@ public:
     //       but it will still contain the "moved-from" state of the object it previously contained.
     Variant(Variant&& old)
         : Detail::MergeAndDeduplicatePacks<Detail::VariantConstructors<Ts, Variant<Ts...>>...>()
+        , m_data {}
         , m_index(old.m_index)
     {
         Helper::move_(old.m_index, old.m_data, m_data);
@@ -250,20 +245,18 @@ public:
 
     using Detail::MergeAndDeduplicatePacks<Detail::VariantConstructors<Ts, Variant<Ts...>>...>::MergeAndDeduplicatePacks;
 
-    template<typename T>
-    void set(T&& t) requires(index_of<T>() != invalid_index)
+    template<typename T, typename StrippedT = RemoveCV<RemoveReference<T>>>
+    void set(T&& t) requires(index_of<StrippedT>() != invalid_index)
     {
-        using StrippedT = RemoveCV<RemoveReference<T>>;
         constexpr auto new_index = index_of<StrippedT>();
         Helper::delete_(m_index, m_data);
         new (m_data) StrippedT(forward<T>(t));
         m_index = new_index;
     }
 
-    template<typename T>
-    void set(T&& t, Detail::VariantNoClearTag)
+    template<typename T, typename StrippedT = RemoveCV<RemoveReference<T>>>
+    void set(T&& t, Detail::VariantNoClearTag) requires(index_of<StrippedT>() != invalid_index)
     {
-        using StrippedT = RemoveCV<RemoveReference<T>>;
         constexpr auto new_index = index_of<StrippedT>();
         new (m_data) StrippedT(forward<T>(t));
         m_index = new_index;
@@ -355,12 +348,21 @@ private:
     using Helper = Detail::Variant<IndexType, 0, Ts...>;
     using VisitHelper = Detail::VisitImpl<IndexType, Ts...>;
 
+    template<typename T_, typename U_>
+    friend struct Detail::VariantConstructors;
+
     explicit Variant(IndexType index, Detail::VariantConstructTag)
         : Detail::MergeAndDeduplicatePacks<Detail::VariantConstructors<Ts, Variant<Ts...>>...>()
         , m_index(index)
     {
     }
 
+    void clear_without_destruction()
+    {
+        __builtin_memset(m_data, 0, data_size);
+        m_index = invalid_index;
+    }
+
     template<typename... Fs>
     struct Visitor : Fs... {
         Visitor(Fs&&... args)
diff --git a/Tests/AK/TestVariant.cpp b/Tests/AK/TestVariant.cpp
index 28dc92340c5..547ec3db24d 100644
--- a/Tests/AK/TestVariant.cpp
+++ b/Tests/AK/TestVariant.cpp
@@ -172,3 +172,44 @@ TEST_CASE(return_values_by_reference)
     EXPECT_EQ(ref->ref_count(), 1u);
     EXPECT_EQ(value->ref_count(), 1u);
 }
+
+struct HoldsInt {
+    int i;
+};
+struct HoldsFloat {
+    float f;
+};
+
+TEST_CASE(copy_assign)
+{
+    {
+        Variant<int, String, float> the_value { 42.0f };
+
+        VERIFY(the_value.has<float>());
+        EXPECT_EQ(the_value.get<float>(), 42.0f);
+
+        int twelve = 12;
+        the_value = twelve;
+        VERIFY(the_value.has<int>());
+        EXPECT_EQ(the_value.get<int>(), 12);
+
+        the_value = String("Hello, world!");
+        VERIFY(the_value.has<String>());
+        EXPECT_EQ(the_value.get<String>(), "Hello, world!");
+    }
+    {
+        Variant<HoldsInt, String, HoldsFloat> the_value { HoldsFloat { 42.0f } };
+
+        VERIFY(the_value.has<HoldsFloat>());
+        EXPECT_EQ(the_value.get<HoldsFloat>().f, 42.0f);
+
+        HoldsInt twelve { 12 };
+        the_value = twelve;
+        VERIFY(the_value.has<HoldsInt>());
+        EXPECT_EQ(the_value.get<HoldsInt>().i, 12);
+
+        the_value = String("Hello, world!");
+        VERIFY(the_value.has<String>());
+        EXPECT_EQ(the_value.get<String>(), "Hello, world!");
+    }
+}