Fix issues pointed by reviews.

Terry Cojean · Terry Cojean · commit c54b520066a6 · 2020-03-30T12:26:19.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,14 @@ TYPED_TEST(Array, CanBeResized)
 }
 
 
-TYPED_TEST(Array, VewCanBeResetNotResized)
+TYPED_TEST(Array, ViewCanBeResetNotResized)
 {
     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);
+    ASSERT_EQ(view.get_num_elems(), 0);
 }
 
 
@@ -342,6 +345,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 +367,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 +412,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 +442,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 +480,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
@@ -295,13 +295,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->resize_is_allowed()) {
             this->resize_and_reset(other.get_num_elems());
         } else {
             GKO_ENSURE_COMPATIBLE_BOUNDS(other.get_num_elems(),
@@ -322,12 +320,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 +349,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 +380,9 @@ 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.
+     * In a view, this resets the pointer, so it does not point to anything
+     * (`num_elems` will also be set to 0). After a reset of a view, the view
+     can be moved to in order to reuse it if needed.
      *
      * All data stored in the array will be lost.
      *
@@ -400,14 +401,13 @@ class Array {
                                     "gko::Executor (nullptr)");
         }
 
-        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->resize_is_allowed()) {
+            num_elems_ = num_elems;
             data_.reset(exec_->alloc<value_type>(num_elems));
         } else {
-            data_.reset(nullptr);
+            this->clear();
         }
     }
 
@@ -464,6 +464,23 @@ class Array {
         data_ = std::move(tmp.data_);
     }
 
+protected:
+    /**
+     * Tells whether resizing is allowed or not for this instance of Array.
+     *
+     * Views cannot be resized since the data is not owned by the Array which
+     * stores a view. It is also unclear whether custom deleter types should be
+     * allowed to resize as they could be a user-created view-type, therefore
+     * only proper Array which use the `default_deleter` can be resized for now.
+     *
+     * @return whether this Array can be resized or not.
+     */
+    bool resize_is_allowed()
+    {
+        return data_.get_deleter().target_type() == typeid(default_deleter);
+    }
+
+
 private:
     using data_manager =
         std::unique_ptr<value_type[], std::function<void(value_type[])>>;
