Fix SkTArray copy construction

We can't use memcpy for copy construction, even when MEM_COPY == true.

Change-Id: I50eb369f0fbf77e8f0ad5a148c67d46df0d3ab0e
Reviewed-on: https://skia-review.googlesource.com/9487
Commit-Queue: Florin Malita <fmalita@chromium.org>
Reviewed-by: Mike Klein <mtklein@chromium.org>
Reviewed-by: Ben Wagner <bungeman@google.com>

2 files changed, 49 insertions, 19 deletions

diff --git a/include/private/SkTArray.h b/include/private/SkTArray.h
index bd0798ce76..24fa30b029 100644
--- a/include/private/SkTArray.h
+++ b/include/private/SkTArray.h
@@ -15,12 +15,12 @@
 #include <new>
 #include <utility>
 
-/** When MEM_COPY is true T will be bit copied when moved.
-    When MEM_COPY is false, T will be copy constructed / destructed.
+/** When MEM_MOVE is true T will be bit copied when moved.
+    When MEM_MOVE is false, T will be copy constructed / destructed.
     In all cases T will be default-initialized on allocation,
     and its destructor will be called from this object's destructor.
 */
-template <typename T, bool MEM_COPY = false> class SkTArray {
+template <typename T, bool MEM_MOVE = false> class SkTArray {
 public:
     /**
      * Creates an empty array with no initial storage
@@ -364,7 +364,7 @@ public:
         return fItemArray[fCount - i - 1];
     }
 
-    bool operator==(const SkTArray<T, MEM_COPY>& right) const {
+    bool operator==(const SkTArray<T, MEM_MOVE>& right) const {
         int leftCount = this->count();
         if (leftCount != right.count()) {
             return false;
@@ -377,7 +377,7 @@ public:
         return true;
     }
 
-    bool operator!=(const SkTArray<T, MEM_COPY>& right) const {
+    bool operator!=(const SkTArray<T, MEM_MOVE>& right) const {
         return !(*this == right);
     }
 
@@ -447,26 +447,28 @@ private:
     /** In the following move and copy methods, 'dst' is assumed to be uninitialized raw storage.
      *  In the following move methods, 'src' is destroyed leaving behind uninitialized raw storage.
      */
-    template <bool E = MEM_COPY> SK_WHEN(E, void) copy(const T* src) {
-        sk_careful_memcpy(fMemArray, src, fCount * sizeof(T));
+    void copy(const T* src) {
+        // Some types may be trivially copyable, in which case we *could* use memcopy; but
+        // MEM_MOVE == true implies that the type is trivially movable, and not necessarily
+        // trivially copyable (think sk_sp<>).  So short of adding another template arg, we
+        // must be conservative and use copy construction.
+        for (int i = 0; i < fCount; ++i) {
+            new (fItemArray + i) T(src[i]);
+        }
     }
-    template <bool E = MEM_COPY> SK_WHEN(E, void) move(int dst, int src) {
+
+    template <bool E = MEM_MOVE> SK_WHEN(E, void) move(int dst, int src) {
         memcpy(&fItemArray[dst], &fItemArray[src], sizeof(T));
     }
-    template <bool E = MEM_COPY> SK_WHEN(E, void) move(void* dst) {
+    template <bool E = MEM_MOVE> SK_WHEN(E, void) move(void* dst) {
         sk_careful_memcpy(dst, fMemArray, fCount * sizeof(T));
     }
 
-    template <bool E = MEM_COPY> SK_WHEN(!E, void) copy(const T* src) {
-        for (int i = 0; i < fCount; ++i) {
-            new (fItemArray + i) T(src[i]);
-        }
-    }
-    template <bool E = MEM_COPY> SK_WHEN(!E, void) move(int dst, int src) {
+    template <bool E = MEM_MOVE> SK_WHEN(!E, void) move(int dst, int src) {
         new (&fItemArray[dst]) T(std::move(fItemArray[src]));
         fItemArray[src].~T();
     }
-    template <bool E = MEM_COPY> SK_WHEN(!E, void) move(void* dst) {
+    template <bool E = MEM_MOVE> SK_WHEN(!E, void) move(void* dst) {
         for (int i = 0; i < fCount; ++i) {
             new (static_cast<char*>(dst) + sizeof(T) * i) T(std::move(fItemArray[i]));
             fItemArray[i].~T();
@@ -531,10 +533,10 @@ private:
 /**
  * Subclass of SkTArray that contains a preallocated memory block for the array.
  */
-template <int N, typename T, bool MEM_COPY = false>
-class SkSTArray : public SkTArray<T, MEM_COPY> {
+template <int N, typename T, bool MEM_MOVE= false>
+class SkSTArray : public SkTArray<T, MEM_MOVE> {
 private:
-    typedef SkTArray<T, MEM_COPY> INHERITED;
+    typedef SkTArray<T, MEM_MOVE> INHERITED;
 
 public:
     SkSTArray() : INHERITED(&fStorage) {
diff --git a/tests/TArrayTest.cpp b/tests/TArrayTest.cpp
index 675aa33bf8..ee6aabc63a 100644
--- a/tests/TArrayTest.cpp
+++ b/tests/TArrayTest.cpp
@@ -5,6 +5,7 @@
  * found in the LICENSE file.
  */
 
+#include "SkRefCnt.h"
 #include "SkTArray.h"
 #include "Test.h"
 
@@ -117,8 +118,35 @@ static void test_swap(skiatest::Reporter* reporter) {
     test_swap(reporter, arraysMoi, sizes);
 }
 
+template <typename T, bool MEM_MOVE>
+void test_copy_ctor(skiatest::Reporter* reporter, SkTArray<T, MEM_MOVE>&& array) {
+    SkASSERT(array.empty());
+    for (int i = 0; i < 5; ++i) {
+        array.emplace_back(new SkRefCnt);
+        REPORTER_ASSERT(reporter, array.back()->unique());
+    }
+
+    {
+        SkTArray<T, MEM_MOVE> copy(array);
+        for (const auto& ref : array)
+            REPORTER_ASSERT(reporter, !ref->unique());
+        for (const auto& ref : copy)
+            REPORTER_ASSERT(reporter, !ref->unique());
+    }
+
+    for (const auto& ref : array)
+        REPORTER_ASSERT(reporter, ref->unique());
+}
+
 DEF_TEST(TArray, reporter) {
     TestTSet_basic<true>(reporter);
     TestTSet_basic<false>(reporter);
     test_swap(reporter);
+
+    test_copy_ctor(reporter, SkTArray<sk_sp<SkRefCnt>, false>());
+    test_copy_ctor(reporter, SkTArray<sk_sp<SkRefCnt>,  true>());
+    test_copy_ctor(reporter, SkSTArray< 1, sk_sp<SkRefCnt>, false>());
+    test_copy_ctor(reporter, SkSTArray< 1, sk_sp<SkRefCnt>,  true>());
+    test_copy_ctor(reporter, SkSTArray<10, sk_sp<SkRefCnt>, false>());
+    test_copy_ctor(reporter, SkSTArray<10, sk_sp<SkRefCnt>,  true>());
 }