Merge Fix the view memory leak and array/view copy/move.

This fixes some of the discussed issues with arrays. The changes are noted in the list below. Some of these points should still be discussed.

+ Add tests for copy/move of all combinations of array/view.
+ Add a new exception helper macro for testing array dimensions.
+ When copying something into a view, do bound checking and ensure what is
  copied has a compatible amount of data.
+ When copying into an array, the proper amount of data is allocated and copied
   over.
+ When moving a into b, b becomes a and a is invalid afterwards.
+ Resize and reset for views throws an error.

Related PR: #485

core/test/base/array.cpp

@@ -33,6 +33,9 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #include <ginkgo/core/base/array.hpp>
 
 
+#include <algorithm>
+
+
 #include <gtest/gtest.h>
 
 
@@ -284,6 +287,46 @@ TYPED_TEST(Array, CanBeResized)
 }
 
 
+TYPED_TEST(Array, ViewCannotBeResized)
+{
+    TypeParam data[] = {1, 2, 3};
+    auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
+
+    EXPECT_THROW(view.resize_and_reset(1), gko::NotSupported);
+    EXPECT_EQ(view.get_num_elems(), 3);
+    ASSERT_EQ(view.get_data()[0], TypeParam{1});
+}
+
+
+template <typename T>
+class my_null_deleter {
+public:
+    using pointer = T *;
+
+    void operator()(pointer) const noexcept {}
+};
+
+template <typename T>
+class my_null_deleter<T[]> {
+public:
+    using pointer = T[];
+
+    void operator()(pointer) const noexcept {}
+};
+
+
+TYPED_TEST(Array, CustomDeleterCannotBeResized)
+{
+    TypeParam data[] = {1, 2, 3};
+    auto view_custom_deleter = gko::Array<TypeParam>(
+        this->exec, 3, data, my_null_deleter<TypeParam[]>{});
+
+    EXPECT_THROW(view_custom_deleter.resize_and_reset(1), gko::NotSupported);
+    EXPECT_EQ(view_custom_deleter.get_num_elems(), 3);
+    ASSERT_EQ(view_custom_deleter.get_data()[0], TypeParam{1});
+}
+
+
 TYPED_TEST(Array, CanBeAssignedAnExecutor)
 {
     gko::Array<TypeParam> a;
@@ -304,16 +347,182 @@ TYPED_TEST(Array, ChangesExecutors)
 }
 
 
-TYPED_TEST(Array, CanCreateView)
+TYPED_TEST(Array, ViewModifiesOriginalData)
 {
     TypeParam data[] = {1, 2, 3};
+    auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
+
+    TypeParam new_data[] = {5, 4, 2};
+    std::copy(new_data, new_data + 3, view.get_data());
+
+    EXPECT_EQ(data[0], TypeParam{5});
+    EXPECT_EQ(data[1], TypeParam{4});
+    EXPECT_EQ(data[2], TypeParam{2});
+    ASSERT_EQ(view.get_num_elems(), 3);
+}
+
+
+TYPED_TEST(Array, CopyArrayToArray)
+{
+    gko::Array<TypeParam> array(this->exec, {1, 2, 3});
+    gko::Array<TypeParam> array2(this->exec, {5, 4, 2, 1});
+
+    array = array2;
+
+    EXPECT_EQ(array.get_data()[0], TypeParam{5});
+    EXPECT_EQ(array.get_data()[1], TypeParam{4});
+    EXPECT_EQ(array.get_data()[2], TypeParam{2});
+    EXPECT_EQ(array.get_data()[3], TypeParam{1});
+    EXPECT_EQ(array.get_num_elems(), 4);
+    EXPECT_NE(array.get_data(), array2.get_data());
+    ASSERT_EQ(array2.get_num_elems(), 4);
+}
+
 
+TYPED_TEST(Array, CopyViewToView)
+{
+    TypeParam data[] = {1, 2, 3};
     auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
-    view = gko::Array<TypeParam>{this->exec, {5, 4, 2}};
+    TypeParam data2[] = {5, 4, 2};
+    auto view2 = gko::Array<TypeParam>::view(this->exec, 3, data2);
+    TypeParam data_size4[] = {5, 4, 2, 1};
+    auto view_size4 = gko::Array<TypeParam>::view(this->exec, 4, data_size4);
+
+    view = view2;
+    view2.get_data()[0] = 2;
 
     EXPECT_EQ(data[0], TypeParam{5});
     EXPECT_EQ(data[1], TypeParam{4});
     EXPECT_EQ(data[2], TypeParam{2});
+    EXPECT_EQ(view.get_num_elems(), 3);
+    EXPECT_EQ(view2.get_num_elems(), 3);
+    EXPECT_EQ(view2.get_data()[0], TypeParam{2});
+    ASSERT_THROW(view2 = view_size4, gko::OutOfBoundsError);
+}
+
+
+TYPED_TEST(Array, CopyViewToArray)
+{
+    TypeParam data[] = {1, 2, 3, 4};
+    auto view = gko::Array<TypeParam>::view(this->exec, 4, data);
+    gko::Array<TypeParam> array(this->exec, {5, 4, 2});
+
+    array = view;
+    view.get_data()[0] = 2;
+
+    EXPECT_EQ(array.get_data()[0], TypeParam{1});
+    EXPECT_EQ(array.get_data()[1], TypeParam{2});
+    EXPECT_EQ(array.get_data()[2], TypeParam{3});
+    EXPECT_EQ(array.get_data()[3], TypeParam{4});
+    EXPECT_EQ(array.get_num_elems(), 4);
+    ASSERT_EQ(view.get_num_elems(), 4);
+}
+
+
+TYPED_TEST(Array, CopyArrayToView)
+{
+    TypeParam data[] = {1, 2, 3};
+    auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
+    gko::Array<TypeParam> array_size2(this->exec, {5, 4});
+    gko::Array<TypeParam> array_size4(this->exec, {5, 4, 2, 1});
+
+    view = array_size2;
+
+    EXPECT_EQ(data[0], TypeParam{5});
+    EXPECT_EQ(data[1], TypeParam{4});
+    EXPECT_EQ(data[2], TypeParam{3});
+    EXPECT_EQ(view.get_num_elems(), 3);
+    EXPECT_EQ(array_size2.get_num_elems(), 2);
+    ASSERT_THROW(view = array_size4, gko::OutOfBoundsError);
+}
+
+
+TYPED_TEST(Array, MoveArrayToArray)
+{
+    gko::Array<TypeParam> array(this->exec, {1, 2, 3});
+    gko::Array<TypeParam> array2(this->exec, {5, 4, 2, 1});
+    auto data2 = array2.get_data();
+
+    array = std::move(array2);
+
+    EXPECT_EQ(array.get_data(), data2);
+    EXPECT_EQ(array.get_data()[0], TypeParam{5});
+    EXPECT_EQ(array.get_data()[1], TypeParam{4});
+    EXPECT_EQ(array.get_data()[2], TypeParam{2});
+    EXPECT_EQ(array.get_data()[3], TypeParam{1});
+    EXPECT_EQ(array.get_num_elems(), 4);
+    EXPECT_EQ(array2.get_data(), nullptr);
+    ASSERT_EQ(array2.get_num_elems(), 0);
+}
+
+
+TYPED_TEST(Array, MoveViewToView)
+{
+    TypeParam data[] = {1, 2, 3, 4};
+    auto view = gko::Array<TypeParam>::view(this->exec, 4, data);
+    TypeParam data2[] = {5, 4, 2};
+    auto view2 = gko::Array<TypeParam>::view(this->exec, 3, data2);
+
+    view = std::move(view2);
+
+    EXPECT_EQ(view.get_data(), data2);
+    EXPECT_EQ(view.get_data()[0], TypeParam{5});
+    EXPECT_EQ(view.get_data()[1], TypeParam{4});
+    EXPECT_EQ(view.get_data()[2], TypeParam{2});
+    EXPECT_EQ(view.get_num_elems(), 3);
+    EXPECT_EQ(view2.get_data(), nullptr);
+    EXPECT_EQ(view2.get_num_elems(), 0);
+    EXPECT_NE(data, nullptr);
+    EXPECT_EQ(data[0], TypeParam{1});
+    EXPECT_EQ(data[1], TypeParam{2});
+    EXPECT_EQ(data[2], TypeParam{3});
+    ASSERT_EQ(data[3], TypeParam{4});
+}
+
+
+TYPED_TEST(Array, MoveViewToArray)
+{
+    TypeParam data[] = {1, 2, 3, 4};
+    gko::Array<TypeParam> array(this->exec, {5, 4, 2});
+    auto view = gko::Array<TypeParam>::view(this->exec, 4, data);
+
+    array = std::move(view);
+
+    EXPECT_EQ(array.get_data(), data);
+    EXPECT_EQ(array.get_data()[0], TypeParam{1});
+    EXPECT_EQ(array.get_data()[1], TypeParam{2});
+    EXPECT_EQ(array.get_data()[2], TypeParam{3});
+    EXPECT_EQ(array.get_data()[3], TypeParam{4});
+    EXPECT_EQ(array.get_num_elems(), 4);
+    EXPECT_EQ(data[0], TypeParam{1});
+    EXPECT_EQ(data[1], TypeParam{2});
+    EXPECT_EQ(data[2], TypeParam{3});
+    EXPECT_EQ(data[3], TypeParam{4});
+    EXPECT_EQ(view.get_data(), nullptr);
+    ASSERT_EQ(view.get_num_elems(), 0);
+}
+
+
+TYPED_TEST(Array, MoveArrayToView)
+{
+    TypeParam data[] = {1, 2, 3};
+    auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
+    gko::Array<TypeParam> array_size2(this->exec, {5, 4});
+    gko::Array<TypeParam> array_size4(this->exec, {5, 4, 2, 1});
+    auto size2_ptr = array_size2.get_data();
+    auto size4_ptr = array_size4.get_data();
+
+    view = std::move(array_size2);
+
+    EXPECT_EQ(view.get_data()[0], TypeParam{5});
+    EXPECT_EQ(view.get_data()[1], TypeParam{4});
+    EXPECT_EQ(view.get_num_elems(), 2);
+    EXPECT_NE(view.get_data(), data);
+    EXPECT_EQ(view.get_data(), size2_ptr);
+    EXPECT_NO_THROW(view = std::move(array_size4));
+    EXPECT_EQ(view.get_data(), size4_ptr);
+    EXPECT_EQ(array_size2.get_data(), nullptr);
+    ASSERT_EQ(array_size2.get_num_elems(), 0);
 }
 
 

include/ginkgo/core/base/array.hpp

@@ -41,6 +41,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 
 #include <ginkgo/core/base/exception.hpp>
+#include <ginkgo/core/base/exception_helpers.hpp>
 #include <ginkgo/core/base/executor.hpp>
 #include <ginkgo/core/base/types.hpp>
 #include <ginkgo/core/base/utils.hpp>
@@ -254,7 +255,8 @@ class Array {
      * Creates an Array from existing memory.
      *
      * The Array does not take ownership of the memory, and will not deallocate
-     * it once it goes out of scope.
+     * it once it goes out of scope. This array type cannot use the function
+     * `resize_and_reset` since it does not own the data it should resize.
      *
      * @param exec  executor where `data` is located
      * @param num_elems  number of elements in `data`
@@ -269,7 +271,10 @@ class Array {
     }
 
     /**
-     * Copies data from another array.
+     * Copies data from another array or view. In the case of an array target,
+     * the array is resized to match the source's size. In the case of a view
+     * target, if the dimensions are not compatible a gko::OutOfBoundsError is
+     * thrown.
      *
      * This does not invoke the constructors of the elements, instead they are
      * copied as POD types.
@@ -291,17 +296,39 @@ class Array {
             data_ = data_manager{nullptr, other.data_.get_deleter()};
         }
         if (other.get_executor() == nullptr) {
-            this->resize_and_reset(0);
+            this->clear();
             return *this;
         }
-        this->resize_and_reset(other.get_num_elems());
-        exec_->copy_from(other.get_executor().get(), num_elems_,
+
+        if (this->is_owning()) {
+            this->resize_and_reset(other.get_num_elems());
+        } else {
+            GKO_ENSURE_COMPATIBLE_BOUNDS(other.get_num_elems(),
+                                         this->num_elems_);
+        }
+        exec_->copy_from(other.get_executor().get(), other.get_num_elems(),
                          other.get_const_data(), this->get_data());
         return *this;
     }
 
     /**
-     * Moves data from another array.
+     * Moves data from another array or view. Only the pointer and deleter type
+     * change, a copy only happens when targeting another executor's data. This
+     * means that in the following situation:
+     * ```cpp
+     *   gko::Array<int> a; // an existing array or view
+     *   gko::Array<int> b; // an existing array or view
+     *   b = std::move(a);
+     * ```
+     * Depending on whether `a` and `b` are array or view, this happens:
+     * + `a` and `b` are views, `b` becomes the only valid view of `a`;
+     * + `a` and `b` are arrays, `b` becomes the only valid array of `a`;
+     * + `a` is a view and `b` is an array, `b` frees its data and becomes the
+     *    only valid view of `a` ();
+     * + `a` is an array and `b` is a view, `b` becomes the only valid array
+     *    of `a`.
+     *
+     * In all the previous cases, `a` becomes invalid (e.g., a `nullptr`).
      *
      * This does not invoke the constructors of the elements, instead they are
      * copied as POD types.
@@ -323,17 +350,17 @@ class Array {
             data_ = data_manager{nullptr, other.data_.get_deleter()};
         }
         if (other.get_executor() == nullptr) {
-            this->resize_and_reset(0);
+            this->clear();
             return *this;
         }
-        if (exec_ == other.get_executor() &&
-            data_.get_deleter().target_type() != typeid(view_deleter)) {
-            // same device and not a view, only move the pointer
+        if (exec_ == other.get_executor()) {
+            // same device, only move the pointer
             using std::swap;
             swap(data_, other.data_);
             swap(num_elems_, other.num_elems_);
+            other.clear();
         } else {
-            // different device or a view, copy the data
+            // different device, copy the data
             *this = other;
         }
         return *this;
@@ -354,6 +381,8 @@ class Array {
 
     /**
      * Resizes the array so it is able to hold the specified number of elements.
+     * For a view and other non-owning Array types, this throws an exception
+     * since these types cannot be resized.
      *
      * All data stored in the array will be lost.
      *
@@ -371,11 +400,16 @@ class Array {
             throw gko::NotSupported(__FILE__, __LINE__, __func__,
                                     "gko::Executor (nullptr)");
         }
-        num_elems_ = num_elems;
-        if (num_elems > 0) {
+        if (!this->is_owning()) {
+            throw gko::NotSupported(__FILE__, __LINE__, __func__,
+                                    "Non owning gko::Array cannot be resized.");
+        }
+
+        if (num_elems > 0 && this->is_owning()) {
+            num_elems_ = num_elems;
             data_.reset(exec_->alloc<value_type>(num_elems));
         } else {
-            data_.reset(nullptr);
+            this->clear();
         }
     }
 
@@ -432,6 +466,23 @@ class Array {
         data_ = std::move(tmp.data_);
     }
 
+    /**
+     * Tells whether this Array owns its data or not.
+     *
+     * Views do not own their data and this has multiple implications. They
+     * cannot be resized since the data is not owned by the Array which stores a
+     * view. It is also unclear whether custom deleter types are owning types as
+     * they could be a user-created view-type, therefore only proper Array which
+     * use the `default_deleter` are considered owning types.
+     *
+     * @return whether this Array can be resized or not.
+     */
+    bool is_owning()
+    {
+        return data_.get_deleter().target_type() == typeid(default_deleter);
+    }
+
+
 private:
     using data_manager =
         std::unique_ptr<value_type[], std::function<void(value_type[])>>;