SkTArray movable and swap for move only elements.

SkTArray cannot currently contain move only elements because its swap
currently requires the SkTArray to be copyable. This makes SkTArray
movable and makes its swap move instead of copy.

Review URL: https://codereview.chromium.org/1904663004

2 files changed, 76 insertions, 39 deletions

diff --git a/include/private/SkTArray.h b/include/private/SkTArray.h
index d12ab1af05..043784f093 100644
--- a/include/private/SkTArray.h
+++ b/include/private/SkTArray.h
@@ -38,14 +38,20 @@ public:
      * elements.
      */
     explicit SkTArray(int reserveCount) {
-        this->init(NULL, 0, NULL, reserveCount);
+        this->init(0, NULL, reserveCount);
     }
 
     /**
      * Copies one array to another. The new array will be heap allocated.
      */
-    explicit SkTArray(const SkTArray& array) {
-        this->init(array.fItemArray, array.fCount, NULL, 0);
+    explicit SkTArray(const SkTArray& that) {
+        this->init(that.fCount, NULL, 0);
+        this->copy(that.fItemArray);
+    }
+    explicit SkTArray(SkTArray&& that) {
+        this->init(that.fCount, NULL, 0);
+        that.move(fMemArray);
+        that.fCount = 0;
     }
 
     /**
@@ -54,20 +60,32 @@ public:
      * when you really want the (void*, int) version.
      */
     SkTArray(const T* array, int count) {
-        this->init(array, count, NULL, 0);
+        this->init(count, NULL, 0);
+        this->copy(array);
     }
 
     /**
      * assign copy of array to this
      */
-    SkTArray& operator =(const SkTArray& array) {
+    SkTArray& operator =(const SkTArray& that) {
+        for (int i = 0; i < fCount; ++i) {
+            fItemArray[i].~T();
+        }
+        fCount = 0;
+        this->checkRealloc(that.count());
+        fCount = that.count();
+        this->copy(that.fItemArray);
+        return *this;
+    }
+    SkTArray& operator =(SkTArray&& that) {
         for (int i = 0; i < fCount; ++i) {
             fItemArray[i].~T();
         }
         fCount = 0;
-        this->checkRealloc((int)array.count());
-        fCount = array.count();
-        this->copy(static_cast<const T*>(array.fMemArray));
+        this->checkRealloc(that.count());
+        fCount = that.count();
+        that.move(fMemArray);
+        that.fCount = 0;
         return *this;
     }
 
@@ -93,7 +111,7 @@ public:
         for (int i = 0; i < fCount; ++i) {
             fItemArray[i].~T();
         }
-        // set fCount to 0 before calling checkRealloc so that no copy cons. are called.
+        // Set fCount to 0 before calling checkRealloc so that no elements are moved.
         fCount = 0;
         this->checkRealloc(n);
         fCount = n;
@@ -109,8 +127,8 @@ public:
         for (int i = 0; i < fCount; ++i) {
             fItemArray[i].~T();
         }
-        int delta = count - fCount;
-        this->checkRealloc(delta);
+        fCount = 0;
+        this->checkRealloc(count);
         fCount = count;
         this->copy(array);
     }
@@ -264,9 +282,9 @@ public:
             SkTSwap(fAllocCount, that->fAllocCount);
         } else {
             // This could be more optimal...
-            SkTArray copy(*that);
-            *that = *this;
-            *this = copy;
+            SkTArray copy(std::move(*that));
+            *that = std::move(*this);
+            *this = std::move(copy);
         }
     }
 
@@ -351,7 +369,7 @@ protected:
      */
     template <int N>
     SkTArray(SkAlignedSTStorage<N,T>* storage) {
-        this->init(NULL, 0, storage->get(), N);
+        this->init(0, storage->get(), N);
     }
 
     /**
@@ -361,7 +379,8 @@ protected:
      */
     template <int N>
     SkTArray(const SkTArray& array, SkAlignedSTStorage<N,T>* storage) {
-        this->init(array.fItemArray, array.fCount, storage->get(), N);
+        this->init(array.fCount, storage->get(), N);
+        this->copy(array.fItemArray);
     }
 
     /**
@@ -371,11 +390,11 @@ protected:
      */
     template <int N>
     SkTArray(const T* array, int count, SkAlignedSTStorage<N,T>* storage) {
-        this->init(array, count, storage->get(), N);
+        this->init(count, storage->get(), N);
+        this->copy(array);
     }
 
-    void init(const T* array, int count,
-              void* preAllocStorage, int preAllocOrReserveCount) {
+    void init(int count, void* preAllocStorage, int preAllocOrReserveCount) {
         SkASSERT(count >= 0);
         SkASSERT(preAllocOrReserveCount >= 0);
         fCount              = count;
@@ -391,8 +410,6 @@ protected:
             fAllocCount = SkMax32(fCount, fReserveCount);
             fMemArray = sk_malloc_throw(fAllocCount * sizeof(T));
         }
-
-        this->copy(array);
     }
 
 private:
@@ -405,7 +422,7 @@ private:
     template <bool E = MEM_COPY> 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(char* dst) {
+    template <bool E = MEM_COPY> SK_WHEN(E, void) move(void* dst) {
         sk_careful_memcpy(dst, fMemArray, fCount * sizeof(T));
     }
 
@@ -418,9 +435,9 @@ private:
         new (&fItemArray[dst]) T(std::move(fItemArray[src]));
         fItemArray[src].~T();
     }
-    template <bool E = MEM_COPY> SK_WHEN(!E, void) move(char* dst) {
+    template <bool E = MEM_COPY> SK_WHEN(!E, void) move(void* dst) {
         for (int i = 0; i < fCount; ++i) {
-            new (dst + sizeof(T) * i) T(std::move(fItemArray[i]));
+            new (static_cast<char*>(dst) + sizeof(T) * i) T(std::move(fItemArray[i]));
             fItemArray[i].~T();
         }
     }
@@ -453,12 +470,12 @@ private:
         if (newAllocCount != fAllocCount) {
 
             fAllocCount = newAllocCount;
-            char* newMemArray;
+            void* newMemArray;
 
             if (fAllocCount == fReserveCount && fPreAllocMemArray) {
-                newMemArray = (char*) fPreAllocMemArray;
+                newMemArray = fPreAllocMemArray;
             } else {
-                newMemArray = (char*) sk_malloc_throw(fAllocCount*sizeof(T));
+                newMemArray = sk_malloc_throw(fAllocCount*sizeof(T));
             }
 
             this->move(newMemArray);
diff --git a/tests/TArrayTest.cpp b/tests/TArrayTest.cpp
index abf1075bc5..675aa33bf8 100644
--- a/tests/TArrayTest.cpp
+++ b/tests/TArrayTest.cpp
@@ -58,15 +58,10 @@ static void TestTSet_basic(skiatest::Reporter* reporter) {
     // {0, 3, 2 }
 }
 
-static void test_swap(skiatest::Reporter* reporter) {
-    SkTArray<int> arr;
-    SkSTArray< 5, int> arr5;
-    SkSTArray<10, int> arr10;
-    SkSTArray<20, int> arr20;
-
-    SkTArray<int>* arrays[] = { &arr, &arr5, &arr10, &arr20 };
-    int sizes[] = {0, 1, 5, 10, 15, 20, 25};
-
+template <typename T> static void test_swap(skiatest::Reporter* reporter,
+                                            SkTArray<T>* (&arrays)[4],
+                                            int (&sizes)[7])
+{
     for (auto a : arrays) {
     for (auto b : arrays) {
         if (a == b) {
@@ -87,16 +82,41 @@ static void test_swap(skiatest::Reporter* reporter) {
             REPORTER_ASSERT(reporter, a->count() == sizeB);
 
             curr = 0;
-            for (int x : *b) { REPORTER_ASSERT(reporter, x == curr++); }
-            for (int x : *a) { REPORTER_ASSERT(reporter, x == curr++); }
+            for (auto&& x : *b) { REPORTER_ASSERT(reporter, x == curr++); }
+            for (auto&& x : *a) { REPORTER_ASSERT(reporter, x == curr++); }
 
             a->swap(a);
             curr = sizeA;
-            for (int x : *a) { REPORTER_ASSERT(reporter, x == curr++); }
+            for (auto&& x : *a) { REPORTER_ASSERT(reporter, x == curr++); }
         }}
     }}
 }
 
+static void test_swap(skiatest::Reporter* reporter) {
+    int sizes[] = {0, 1, 5, 10, 15, 20, 25};
+
+    SkTArray<int> arr;
+    SkSTArray< 5, int> arr5;
+    SkSTArray<10, int> arr10;
+    SkSTArray<20, int> arr20;
+    SkTArray<int>* arrays[] = { &arr, &arr5, &arr10, &arr20 };
+    test_swap(reporter, arrays, sizes);
+
+    struct MoveOnlyInt {
+        MoveOnlyInt(int i) : fInt(i) {}
+        MoveOnlyInt(MoveOnlyInt&& that) : fInt(that.fInt) {}
+        bool operator==(int i) { return fInt == i; }
+        int fInt;
+    };
+
+    SkTArray<MoveOnlyInt> moi;
+    SkSTArray< 5, MoveOnlyInt> moi5;
+    SkSTArray<10, MoveOnlyInt> moi10;
+    SkSTArray<20, MoveOnlyInt> moi20;
+    SkTArray<MoveOnlyInt>* arraysMoi[] = { &moi, &moi5, &moi10, &moi20 };
+    test_swap(reporter, arraysMoi, sizes);
+}
+
 DEF_TEST(TArray, reporter) {
     TestTSet_basic<true>(reporter);
     TestTSet_basic<false>(reporter);