[core] Move should not call destructorsupstream/fabian-cp-14352

3 files changed, 108 insertions, 18 deletions

diff --git a/include/mbgl/style/conversion_impl.hpp b/include/mbgl/style/conversion_impl.hpp
index 27b2ee1917..0551187f6a 100644
--- a/include/mbgl/style/conversion_impl.hpp
+++ b/include/mbgl/style/conversion_impl.hpp
@@ -89,29 +89,20 @@ public:
         new (static_cast<void*>(&storage)) std::decay_t<T>(std::forward<T>(value));
     }
 
-    Convertible(Convertible&& v)
-        : vtable(v.vtable)
-    {
-        if (vtable) {
-            vtable->move(std::move(v.storage), this->storage);
-        }
+    Convertible(Convertible&& v) : vtable(v.vtable) {
+        vtable->move(std::move(v.storage), storage);
     }
 
     ~Convertible() {
-        if (vtable) {
-            vtable->destroy(storage);
-        }
+        vtable->destroy(storage);
     }
 
     Convertible& operator=(Convertible&& v) {
-        if (vtable) {
+        if (this != &v) {
             vtable->destroy(storage);
+            vtable = v.vtable;
+            vtable->move(std::move(v.storage), storage);
         }
-        vtable = v.vtable;
-        if (vtable) {
-            vtable->move(std::move(v.storage), this->storage);
-        }
-        v.vtable = nullptr;
         return *this;
     }
 
@@ -235,9 +226,7 @@ private:
         using Traits = ConversionTraits<T>;
         static VTable vtable = {
             [] (Storage&& src, Storage& dest) {
-                auto srcValue = reinterpret_cast<T&&>(src);
-                new (static_cast<void*>(&dest)) T(std::move(srcValue));
-                srcValue.~T();
+                new (static_cast<void*>(&dest)) T(reinterpret_cast<T&&>(src));
             },
             [] (Storage& s) {
                 reinterpret_cast<T&>(s).~T();
diff --git a/test/style/conversion/conversion_impl.test.cpp b/test/style/conversion/conversion_impl.test.cpp
new file mode 100644
index 0000000000..633d77886f
--- /dev/null
+++ b/test/style/conversion/conversion_impl.test.cpp
@@ -0,0 +1,100 @@
+#include <mbgl/test/util.hpp>
+#include <mbgl/style/conversion_impl.hpp>
+
+namespace mbgl {
+namespace style {
+namespace conversion {
+
+class MockConvertible {
+public:
+    MockConvertible() = default;
+    MockConvertible(int* counter_) : counter(counter_) {}
+
+    MockConvertible(MockConvertible&& other) noexcept : counter(other.counter) {}
+
+    ~MockConvertible() {
+        ++*counter;
+    }
+
+    int* counter = nullptr;
+};
+
+template <>
+class ConversionTraits<MockConvertible> {
+public:
+    static bool isUndefined(const MockConvertible&) {
+        return false;
+    }
+
+    static bool isArray(const MockConvertible&) {
+        return false;
+    }
+
+    static bool isObject(const MockConvertible&) {
+        return false;
+    }
+
+    static std::size_t arrayLength(const MockConvertible&) {
+        return 0u;
+    }
+
+    static MockConvertible arrayMember(const MockConvertible&, std::size_t) {
+        return {};
+    }
+
+    static optional<MockConvertible> objectMember(const MockConvertible&, const char *) {
+        return nullopt;
+    }
+
+    template <class Fn>
+    static optional<Error> eachMember(const MockConvertible&, Fn&&) {
+        return nullopt;
+    }
+
+    static optional<bool> toBool(const MockConvertible&) {
+        return nullopt;
+    }
+
+    static optional<float> toNumber(const MockConvertible&) {
+        return nullopt;
+    }
+
+    static optional<double> toDouble(const MockConvertible&) {
+        return nullopt;
+    }
+
+    static optional<std::string> toString(const MockConvertible&) {
+        return nullopt;
+    }
+
+    static optional<mbgl::Value> toValue(const MockConvertible&) {
+        return nullopt;
+    }
+
+    static optional<GeoJSON> toGeoJSON(const MockConvertible&, Error&) {
+        return nullopt;
+    }
+};
+
+} // namespace conversion
+} // namespace style
+} // namespace mbgl
+
+using namespace mbgl;
+using namespace mbgl::style;
+using namespace mbgl::style::conversion;
+
+TEST(Conversion, Move) {
+    int dtorCounter = 0;
+
+    {
+        MockConvertible mock(&dtorCounter);
+        Convertible a(std::move(mock));
+        Convertible b(std::move(a));
+        a = std::move(b);
+        Convertible* c = &a;
+        a = std::move(*c);
+    }
+
+    ASSERT_EQ(dtorCounter, 4);
+}
diff --git a/test/test-files.json b/test/test-files.json
index 498237e072..f958fba0d3 100644
--- a/test/test-files.json
+++ b/test/test-files.json
@@ -49,6 +49,7 @@
         "test/storage/online_file_source.test.cpp",
         "test/storage/resource.test.cpp",
         "test/storage/sqlite.test.cpp",
+        "test/style/conversion/conversion_impl.test.cpp",
         "test/style/conversion/function.test.cpp",
         "test/style/conversion/geojson_options.test.cpp",
         "test/style/conversion/layer.test.cpp",