From c94067d9ae30d59aed8a829c40afd10e4f791903 Mon Sep 17 00:00:00 2001
From: John Firebaugh <john.firebaugh@gmail.com>
Date: Mon, 18 Jun 2018 16:24:58 -0700
Subject: [core] Fix stylistic issues in unique_any

* Eliminate unnecessary temporary in VTableStack::move
* Make move consistent: it destructs the src, not the dest, which is always empty
* delete doesn't need a null guard
* Conversions to void* don't need a cast
---
 include/mbgl/util/unique_any.hpp | 16 ++++++----------
 test/util/unique_any.test.cpp    | 40 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 42 insertions(+), 14 deletions(-)

diff --git a/include/mbgl/util/unique_any.hpp b/include/mbgl/util/unique_any.hpp
index d488930a03..c7dc8b38ea 100644
--- a/include/mbgl/util/unique_any.hpp
+++ b/include/mbgl/util/unique_any.hpp
@@ -135,15 +135,12 @@ private:
     template <typename ValueType>
     struct VTableHeap : public VTable {
         void move(Storage&& src, Storage& dest) override {
-            destroy(dest);
             dest.dynamic = src.dynamic;
+            src.dynamic = nullptr;
         }
 
         void destroy(Storage& s) override {
-            if (s.dynamic) {
-                delete reinterpret_cast<ValueType*>(s.dynamic);
-            }
-            s.dynamic = nullptr;
+            delete reinterpret_cast<ValueType*>(s.dynamic);
         }
 
         const std::type_info& type() override {
@@ -154,9 +151,8 @@ private:
     template <typename ValueType>
     struct VTableStack : public VTable {
         void move(Storage&& src, Storage& dest) override {
-            auto srcValue = reinterpret_cast<ValueType&&>(src.stack);
-            new (static_cast<void*>(&dest.stack)) ValueType(std::move(srcValue));
-            srcValue.~ValueType();
+            new (&dest.stack) ValueType(std::move(reinterpret_cast<ValueType&>(src.stack)));
+            destroy(src);
         }
 
         void destroy(Storage& s) override {
@@ -178,13 +174,13 @@ private:
     template <typename ValueType, typename _Vt>
     std::enable_if_t<AllocateOnStack<_Vt>::value>
     createStorage(ValueType&& value) {
-        new (static_cast<void*>(&storage.stack)) _Vt(std::forward<ValueType>(value));
+        new (&storage.stack) _Vt(std::forward<ValueType>(value));
     }
 
     template <typename ValueType, typename _Vt>
     std::enable_if_t<!AllocateOnStack<_Vt>::value>
     createStorage(ValueType&& value) {
-        storage.dynamic = static_cast<void*>(new _Vt(std::forward<ValueType>(value)));
+        storage.dynamic = new _Vt(std::forward<ValueType>(value));
     }
 
     template <typename ValueType>
diff --git a/test/util/unique_any.test.cpp b/test/util/unique_any.test.cpp
index 9357b9c0ec..9b622cd284 100644
--- a/test/util/unique_any.test.cpp
+++ b/test/util/unique_any.test.cpp
@@ -10,15 +10,14 @@ public:
         str[0] = 'a';
     }
     
-    TestType(unique_any& p) : TestType() {
-        p = std::unique_ptr<TestType>(this);
-    }
-
     //Detect moves
     TestType(TestType&& t): i1(t.i1+1), i2(t.i2+2) {
         str[0] = t.str[0]+1;
     }
 
+    TestType(const TestType&) = delete;
+    TestType& operator=(const TestType&) = delete;
+
     int i1;
     int i2;
     char str[256];
@@ -88,6 +87,39 @@ TEST(UniqueAny, BasicTypes_Move) {
 
 }
 
+TEST(UniqueAny, SmallType) {
+    struct T {
+        T(int32_t* p_) : p(p_) {
+            (*p)++;
+        }
+
+        T(T&& t) noexcept : p(t.p) {
+            (*p)++;
+        }
+
+        ~T() {
+            (*p)--;
+        }
+
+        T(const T&) = delete;
+        T& operator=(const T&) = delete;
+
+        int32_t* p;
+    };
+
+    int32_t p = 0;
+
+    {
+        unique_any u1 = unique_any(T(&p));
+        EXPECT_EQ(p, 1);
+
+        auto u2(std::move(u1));
+        EXPECT_EQ(p, 1);
+    }
+
+    EXPECT_EQ(p, 0);
+}
+
 TEST(UniqueAny, LargeType) {
     TestType t1;
     unique_any u1 = unique_any(std::move(t1));
-- 
cgit v1.2.1