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

+ 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 unsure what is
  copied has a compatible amount of data. To discuss.
+ When moving a into b, b becomes a and a is invalid afterwards.
+ Resize and reset for views reset the pointer to nullptr and do not allocate
  anything. To discuss.

core/test/base/array.cpp

@@ -284,6 +284,17 @@ TYPED_TEST(Array, CanBeResized)
 }
 
 
+TYPED_TEST(Array, VewCanBeResetNotResized)
+{
+ TypeParam data[] = {1, 2, 3};
+ auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
+ view.resize_and_reset(1);
+
+ EXPECT_EQ(view.get_const_data(), nullptr);
+ ASSERT_EQ(view.get_num_elems(), 1);
+}
+
+
 TYPED_TEST(Array, CanBeAssignedAnExecutor)
 {
  gko::Array<TypeParam> a;
@@ -304,16 +315,168 @@ 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);
+ 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);
+ 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});
+
+ array = std::move(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_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);
+ ASSERT_EQ(view2.get_num_elems(), 0);
+}
+
+
+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});
+
+ 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_NO_THROW(view = array_size4);
+ 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>
@@ -269,7 +270,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.
@@ -294,14 +298,36 @@ class Array {
  this->resize_and_reset(0);
  return *this;
  }
- this->resize_and_reset(other.get_num_elems());
- exec_->copy_from(other.get_executor().get(), num_elems_,
+ // We allow resizing only if we are an array, we cannot resize a view!
+ // If we are a view, we do bound checking to ensure the data can be
+ // copied over.
+ if (data_.get_deleter().target_type() != typeid(view_deleter)) {
+ 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 view, `b` becomes the only valid view of `a`
+ * + `a` and `b` are array, `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`
  *
  * This does not invoke the constructors of the elements, instead they are
  * copied as POD types.
@@ -326,14 +352,14 @@ class Array {
  this->resize_and_reset(0);
  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.resize_and_reset(0);
  } else {
- // different device or a view, copy the data
+ // different device, copy the data
  *this = other;
  }
  return *this;
@@ -354,6 +380,8 @@ class Array {
 
  /**
  * Resizes the array so it is able to hold the specified number of elements.
+ * To a view, this only resets the pointer. Afterwards, the only way to use
+ * a view in such a state is to `std::move` another view.
  *
  * All data stored in the array will be lost.
  *
@@ -371,8 +399,12 @@ class Array {
  throw gko::NotSupported(__FILE__, __LINE__, __func__,
  "gko::Executor (nullptr)");
  }
+
  num_elems_ = num_elems;
- if (num_elems > 0) {
+ // For views, we should never allocate any data, all we can do is unset
+ // the pointer.
+ if (num_elems > 0 &&
+ data_.get_deleter().target_type() != typeid(view_deleter)) {
  data_.reset(exec_->alloc<value_type>(num_elems));
  } else {
  data_.reset(nullptr);

include/ginkgo/core/base/exception_helpers.hpp

@@ -454,6 +454,25 @@ inline T ensure_allocated_impl(T ptr, const std::string &file, int line,
  "semi-colon warnings")
 
 
+/**
+ * Ensures that two dimensions have compatible bounds, in particular before a
+ * copy operation. This means the target should have at least as much elements
+ * as the source.
+ *
+ * @param _source the source of the expected copy operation
+ * @param _target the destination of the expected copy operation
+ *
+ * @throw OutOfBoundsError if `_source > _target`
+ */
+#define GKO_ENSURE_COMPATIBLE_BOUNDS(_source, _target) \
+ if (_source > _target) { \
+ throw ::gko::OutOfBoundsError(__FILE__, __LINE__, _source, _target); \
+ } \
+ static_assert(true, \
+ "This assert is used to counter the false positive extra " \
+ "semi-colon warnings")
+
+
 /**
  * Creates a StreamError exception.
  * This macro sets the correct information about the location of the error