Fix issues pointed by reviews.

Terry Cojean · Terry Cojean · commit ad9802bb9449 · 2020-04-01T12:31:51.000+02:00
diff --git a/core/test/base/array.cpp b/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,14 +287,43 @@ TYPED_TEST(Array, CanBeResized)
 }
 
 
-TYPED_TEST(Array, VewCanBeResetNotResized)
+TYPED_TEST(Array, ViewCannotBeResized)
 {
     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);
+    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});
 }
 
 
@@ -342,6 +374,7 @@ TYPED_TEST(Array, CopyArrayToArray)
     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);
 }
 
@@ -363,6 +396,7 @@ TYPED_TEST(Array, CopyViewToView)
     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);
 }
 
@@ -407,9 +441,11 @@ 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});
@@ -435,7 +471,12 @@ TYPED_TEST(Array, MoveViewToView)
     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);
+    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});
 }
 
 
@@ -468,13 +509,18 @@ TYPED_TEST(Array, MoveArrayToView)
     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_NO_THROW(view = array_size4);
+    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);
 }
diff --git a/include/ginkgo/core/base/array.hpp b/include/ginkgo/core/base/array.hpp
@@ -255,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`
@@ -295,13 +296,11 @@ class Array {
             data_ = data_manager{nullptr, other.data_.get_deleter()};
         }
         if (other.get_executor() == nullptr) {
-            this->resize_and_reset(0);
+            this->clear();
             return *this;
         }
-        // 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)) {
+
+        if (this->is_owning()) {
             this->resize_and_reset(other.get_num_elems());
         } else {
             GKO_ENSURE_COMPATIBLE_BOUNDS(other.get_num_elems(),
@@ -322,12 +321,14 @@ class Array {
      *   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`
+     * + `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.
@@ -349,15 +350,15 @@ 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()) {
             // same device, only move the pointer
             using std::swap;
             swap(data_, other.data_);
             swap(num_elems_, other.num_elems_);
-            other.resize_and_reset(0);
+            other.clear();
         } else {
             // different device, copy the data
             *this = other;
@@ -380,8 +381,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.
+     * 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.
      *
@@ -399,15 +400,16 @@ class Array {
             throw gko::NotSupported(__FILE__, __LINE__, __func__,
                                     "gko::Executor (nullptr)");
         }
+        if (!this->is_owning()) {
+            throw gko::NotSupported(__FILE__, __LINE__, __func__,
+                                    "Non owning gko::Array cannot be resized.");
+        }
 
-        num_elems_ = num_elems;
-        // 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)) {
+        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();
         }
     }
 
@@ -464,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[])>>;
