Array: clean-up constructors that take existing data

- get rid of copy_data argument for constructors but use sensible defaults
- add VectorWithOffset::init such that Array doesn't need to know much

src/include/stir/Array.h

@@ -3,7 +3,7 @@
     Copyright (C) 2000 PARAPET partners
     Copyright (C) 2000 - 2011-10-14, Hammersmith Imanet Ltd
     Copyright (C) 2011-07-01 - 2012, Kris Thielemans
-    Copyright (C) 2023, University College London
+    Copyright (C) 2023 - 2024, University College London
     This file is part of STIR.
 
     SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license
@@ -136,19 +136,17 @@ class Array : public NumericVectorWithOffset<Array<num_dimensions - 1, elemT>, e
   //! Construct an Array of given range of indices, elements are initialised to 0
   inline explicit Array(const IndexRange<num_dimensions>&);
 
-  //! Construct an Array from existing contiguous data
+  //! Construct an Array pointing to existing contiguous data
   /*!
-    \arg data_ptr should point to a contiguous block of correct size
-    \arg copy_data if \c false, the constructed Array will essentially be a "view" of the
-       \a data_ptr block. Therefore, any modifications to the array will modify the data at \a data_ptr.
+    \arg data_sptr should point to a contiguous block of correct size.
+    The constructed Array will essentially be a "view" of the
+       \c data_sptr.get() block. Therefore, any modifications to the array will modify the data at \c data_sptr.get().
+    This will be true until the Array is resized.
 
     The C-array \data_ptr will be accessed with the last dimension running fastest
     ("row-major" order).
-
-    \warning When using \a copy_data = \c false, the user is responsible of memory management of
-    the block pointed to by \a data_ptr.
   */
-  inline Array(const IndexRange<num_dimensions>& range, shared_ptr<elemT[]> data_sptr, bool copy_data);
+  inline Array(const IndexRange<num_dimensions>& range, shared_ptr<elemT[]> data_sptr);
 
 #ifndef SWIG
   // swig 2.0.4 gets confused by base_type (due to numeric template arguments)
@@ -387,14 +385,18 @@ class Array<1, elemT> : public NumericVectorWithOffset<elemT, elemT>
 
   //! constructor given an IndexRange<1>, pointing to existing contiguous data
   /*!
-    \arg data_ptr should point to a contiguous block of correct size
-    \arg copy_data if \c false, the constructed Array will essentially be a "view" of the
-       \a data_ptr block. Therefore, any modifications to the array will modify the data at \a data_ptr.
+    \arg data_ptr should point to a contiguous block of correct size.
+    The constructed Array will essentially be a "view" of the
+    \c data_sptr block. Therefore, any modifications to the array will modify the data at \a data_sptr.
+    This will be the case until the Array is resized.
+  */
+  inline Array(const IndexRange<1>& range, shared_ptr<elemT[]> data_sptr);
 
-    \warning When using \a copy_data = \c false, the user is responsible of memory management of
-    the block pointed to by \a data_ptr.
+  //! constructor given an IndexRange<1> from existing contiguous data (will copy)
+  /*!
+    \arg data_ptr should point to a contiguous block of correct size.
   */
-  inline Array(const IndexRange<1>& range, shared_ptr<elemT[]> data_sptr, bool copy_data);
+  inline Array(const IndexRange<1>& range, const elemT* const data_ptr);
 
   //! constructor from basetype
   inline Array(const NumericVectorWithOffset<elemT, elemT>& il);

src/include/stir/Array.inl

@@ -4,7 +4,7 @@
     Copyright (C) 2000 PARAPET partners
     Copyright (C) 2000 - 2011-01-11, Hammersmith Imanet Ltd
     Copyright (C) 2011-07-01 - 2012, Kris Thielemans
-    Copyright (C) 2023, University College London
+    Copyright (C) 2023 - 2024, University College London
     This file is part of STIR.
 
     SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license
@@ -102,15 +102,9 @@ Array<num_dimensions, elemT>::Array(const IndexRange<num_dimensions>& range)
 }
 
 template <int num_dimensions, typename elemT>
-Array<num_dimensions, elemT>::Array(const IndexRange<num_dimensions>& range, shared_ptr<elemT[]> data_sptr, bool copy_data)
+Array<num_dimensions, elemT>::Array(const IndexRange<num_dimensions>& range, shared_ptr<elemT[]> data_sptr)
 {
-  if (copy_data)
-    {
-      this->_allocated_full_data_ptr = std::shared_ptr<elemT[]>(new elemT[range.size_all()]);
-      std::copy(data_sptr.get(), data_sptr.get() + range.size_all(), this->_allocated_full_data_ptr.get());
-    }
-  else
-    this->_allocated_full_data_ptr = data_sptr;
+  this->_allocated_full_data_ptr = data_sptr;
   this->init(range, this->_allocated_full_data_ptr.get(), false);
 }
 
@@ -617,12 +611,14 @@ Array<1, elemT>::Array(const int min_index, const int max_index)
 }
 
 template <class elemT>
-Array<1, elemT>::Array(const IndexRange<1>& range, shared_ptr<elemT[]> data_sptr, bool copy_data)
-{
-  if (!copy_data)
-    this->allocated_memory_sptr = data_sptr;
-  this->init(range, data_sptr.get(), copy_data);
-}
+Array<1, elemT>::Array(const IndexRange<1>& range, shared_ptr<elemT[]> data_sptr)
+    : base_type(range.get_min_index(), range.get_max_index(), data_sptr)
+{}
+
+template <class elemT>
+Array<1, elemT>::Array(const IndexRange<1>& range, const elemT* const data_ptr)
+    : base_type(range.get_min_index(), range.get_max_index(), data_ptr)
+{}
 
 template <class elemT>
 Array<1, elemT>::Array(const base_type& il)

src/include/stir/NumericVectorWithOffset.h

@@ -50,14 +50,7 @@ class NumericVectorWithOffset : public VectorWithOffset<T>
   typedef VectorWithOffset<T> base_type;
 
 public:
-  //! Construct an empty NumericVectorWithOffset
-  inline NumericVectorWithOffset();
-
-  //! Construct a NumericVectorWithOffset of given length
-  inline explicit NumericVectorWithOffset(const int hsz);
-
-  //! Construct a NumericVectorWithOffset of elements with offset \c min_index
-  inline NumericVectorWithOffset(const int min_index, const int max_index);
+  using base_type::base_type;
 
   //! Constructor from an object of this class' base_type
   inline NumericVectorWithOffset(const VectorWithOffset<T>& t);

src/include/stir/NumericVectorWithOffset.inl

@@ -27,21 +27,6 @@
 
 START_NAMESPACE_STIR
 
-template <class T, class NUMBER>
-inline NumericVectorWithOffset<T, NUMBER>::NumericVectorWithOffset()
-    : base_type()
-{}
-
-template <class T, class NUMBER>
-inline NumericVectorWithOffset<T, NUMBER>::NumericVectorWithOffset(const int hsz)
-    : base_type(hsz)
-{}
-
-template <class T, class NUMBER>
-inline NumericVectorWithOffset<T, NUMBER>::NumericVectorWithOffset(const int min_index, const int max_index)
-    : base_type(min_index, max_index)
-{}
-
 template <class T, class NUMBER>
 NumericVectorWithOffset<T, NUMBER>::NumericVectorWithOffset(const VectorWithOffset<T>& t)
     : base_type(t)

src/include/stir/VectorWithOffset.h

@@ -4,7 +4,7 @@
     Copyright (C) 2000 PARAPET partners
     Copyright (C) 2000 - 2007-10-08, Hammersmith Imanet Ltd
     Copyright (C) 2012-06-01 - 2012, Kris Thielemans
-    Copyright (C) 2023, University College London
+    Copyright (C) 2023 - 2024, University College London
     This file is part of STIR.
 
     SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license
@@ -24,6 +24,7 @@
 */
 
 #include "stir/shared_ptr.h"
+#include "stir/deprecated.h"
 #include "boost/iterator/iterator_adaptor.hpp"
 #include "boost/iterator/reverse_iterator.hpp"
 
@@ -138,20 +139,46 @@ class VectorWithOffset
   //! Construct a VectorWithOffset with offset \c min_index (initialised with \c  T())
   inline VectorWithOffset(const int min_index, const int max_index);
 
-  //! Construct a VectorWithOffset of given length using existing data (no initialisation)
-  inline explicit VectorWithOffset(const int hsz, T* const data_ptr, T* const end_of_data_ptr);
-
+#if STIR_VERSION < 070000
   //! Construct a VectorWithOffset of given length pointing to existing data
-  inline VectorWithOffset(const int hsz, T* const data_ptr)
-      : VectorWithOffset(hsz, data_ptr, data_ptr + hsz)
-  {}
+  /*!
+    \warning This refers to the original memory range, so any modifications to this object will modify
+    the original data as well.
+
+    \deprecated
+  */
+  STIR_DEPRECATED VectorWithOffset(const int hsz, T* const data_ptr, T* const end_of_data_ptr);
+
+  //! Construct a VectorWithOffset with offset \c min_index pointing to existing data
+  /*!
+    \warning This refers to the original memory range, so any modifications to this object will modify
+    the original data as well.
+
+    \deprecated
+  */
+  STIR_DEPRECATED inline VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr);
+#endif
 
-  //! Construct a VectorWithOffset with offset \c min_index using existing data (no initialisation)
-  inline VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr);
+  //! Construct a VectorWithOffset of given length from a bare pointer (copying data)
+  VectorWithOffset(const int hsz, const T* const data_ptr);
 
-  //! Construct a VectorWithOffset with offset \c min_index point to existing data
-  inline VectorWithOffset(const int min_index, const int max_index, T* const data_ptr)
-      : VectorWithOffset(min_index, max_index, data_ptr, data_ptr + (max_index - min_index + 1))
+  //! Construct a VectorWithOffset with offset \c min_index from a bare pointer (copying data)
+  inline VectorWithOffset(const int min_index, const int max_index, const T* const data_ptr);
+
+  //! Construct a VectorWithOffset sharing existing data
+  /*!
+    \warning This refers to the original memory range, so any modifications to this object will modify
+    the original data as well.
+  */
+  inline VectorWithOffset(const int min_index, const int max_index, shared_ptr<T[]> data_sptr);
+
+  //! Construct a VectorWithOffset sharing existing data
+  /*!
+    \warning This refers to the original memory range, so any modifications to this object will modify
+    the original data as well.
+  */
+  inline VectorWithOffset(const int sz, shared_ptr<T[]> data_sptr)
+      : VectorWithOffset(0, sz - 1, data_sptr)
   {}
 
   //! copy constructor
@@ -391,8 +418,7 @@ class VectorWithOffset
 
 protected:
   //! pointer to (*this)[0] (taking get_min_index() into account that is).
-  T* num; // TODOKT make private
-  shared_ptr<T[]> allocated_memory_sptr;
+  T* num; // TODO make private
 
   //! Called internally to see if all variables are consistent
   inline void check_state() const;
@@ -411,6 +437,9 @@ class VectorWithOffset
   T* begin_allocated_memory;
   T* end_allocated_memory;
 
+  //! shared_ptr to memory
+  shared_ptr<T[]> allocated_memory_sptr;
+
   //! Default member settings for all constructors
   inline void init();
 

src/include/stir/VectorWithOffset.inl

@@ -4,7 +4,7 @@
     Copyright (C) 2000 PARAPET partners
     Copyright (C) 2000 - 2010-07-01, Hammersmith Imanet Ltd
     Copyright (C) 2012-06-01 - 2012, Kris Thielemans
-    Copyright (C) 2023, University College London
+    Copyright (C) 2023 - 2024, University College London
     This file is part of STIR.
 
     SPDX-License-Identifier: Apache-2.0 AND License-ref-PARAPET-license
@@ -271,24 +271,47 @@ VectorWithOffset<T>::VectorWithOffset(const int min_index, const int max_index)
   this->check_state();
 }
 
-template <class T>
-VectorWithOffset<T>::VectorWithOffset(const int sz, T* const data_ptr, T* const end_of_data_ptr)
-    : VectorWithOffset(0, sz - 1, data_ptr, end_of_data_ptr)
-{}
-
+#if STIR_VERSION < 070000
 template <class T>
 VectorWithOffset<T>::VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr)
     : length(static_cast<unsigned>(max_index - min_index) + 1),
       start(min_index),
       pointer_access(false),
-      allocated_memory_sptr(nullptr)
+      allocated_memory_sptr(nullptr) // we don't own the data
 {
   this->begin_allocated_memory = data_ptr;
   this->end_allocated_memory = end_of_data_ptr;
   this->num = this->begin_allocated_memory - this->start;
   this->check_state();
 }
 
+template <class T>
+VectorWithOffset<T>::VectorWithOffset(const int sz, T* const data_ptr, T* const end_of_data_ptr)
+    : VectorWithOffset(0, sz - 1, data_ptr, end_of_data_ptr)
+{}
+#endif // STIR_VERSION < 070000
+
+template <class T>
+VectorWithOffset<T>::VectorWithOffset(const int min_index, const int max_index, const T* const data_ptr)
+{
+  // first set empty, such that resize() will work ok
+  this->init();
+  // note: need a const_cast, but it's safe because we will copy the data
+  this->init(min_index, max_index, const_cast<T*>(data_ptr), /* copy_data = */ true);
+}
+
+template <class T>
+VectorWithOffset<T>::VectorWithOffset(const int sz, const T* const data_ptr)
+    : VectorWithOffset(0, sz - 1, data_ptr)
+{}
+
+template <class T>
+VectorWithOffset<T>::VectorWithOffset(const int min_index, const int max_index, shared_ptr<T[]> data_sptr)
+{
+  this->allocated_memory_sptr = data_sptr;
+  this->init(min_index, max_index, data_sptr.get(), /* copy_data = */ false);
+}
+
 template <class T>
 VectorWithOffset<T>::VectorWithOffset(VectorWithOffset<T>&& other) noexcept
     : VectorWithOffset()

src/test/test_Array.cxx

@@ -2,7 +2,7 @@
     Copyright (C) 2000 PARAPET partners
     Copyright (C) 2000-2011, Hammersmith Imanet Ltd
     Copyright (C) 2013 Kris Thielemans
-    Copyright (C) 2013, 2020, 2023 University College London
+    Copyright (C) 2013, 2020, 2023, 2024 University College London
 
     This file is part of STIR.
 
@@ -279,30 +279,12 @@ class ArrayTests : public RunTests
   void run_tests() override;
 };
 
-template <typename T>
-class Deleter
-{
-public:
-  Deleter(std::vector<T>& v)
-      : v(v)
-  {}
-  void operator()(T* vptr)
-  {
-    info("in deleter");
-    if (vptr != v.data())
-      error("deleter error");
-    //~v;
-  }
-
-private:
-  std::vector<T>& v;
-};
-
+// helper function to create a shared_ptr that doesn't delete the data (as it's still owned by the vector)
 template <typename T>
 shared_ptr<T[]>
 vec_to_shared(std::vector<T>& v)
 {
-  shared_ptr<T[]> sptr(v.data(), Deleter<T>(v));
+  shared_ptr<T[]> sptr(v.data(), [](auto) {});
   return sptr;
 }
 
@@ -380,8 +362,7 @@ ArrayTests::run_tests()
       {
         std::vector<float> mem(test.get_index_range().size_all());
         std::copy(test.begin_all_const(), test.end_all_const(), mem.begin());
-        // Array<1,float> preallocated(test.get_index_range(), &mem[0], false);
-        Array<1, float> preallocated(test.get_index_range(), vec_to_shared(mem), false);
+        Array<1, float> preallocated(test.get_index_range(), vec_to_shared(mem));
         // shared_ptr<float[]> mem_sptr(new float [test.get_index_range().size_all()]);
         // auto mem = mem_sptr.get();
         // std::copy(test.begin_all_const(), test.end_all_const(), mem);
@@ -419,11 +400,7 @@ ArrayTests::run_tests()
       {
         std::vector<float> mem(test.get_index_range().size_all());
         std::copy(test.begin_all_const(), test.end_all_const(), mem.begin());
-        // Array<1,float> test_from_mem(test.get_index_range(), &mem[0], true);
-        Array<1, float> test_from_mem(test.get_index_range(), vec_to_shared(mem), true);
-        // shared_ptr<float[]> mem(test.get_index_range().size_all());
-        // std::copy(test.begin_all_const(), test.end_all_const(), mem.get());
-        // Array<1,float> preallocated(test.get_index_range(), mem.get(), false);
+        Array<1, float> test_from_mem(test.get_index_range(), reinterpret_cast<const float*>(mem.data()));
         check(test_from_mem.owns_memory_for_data(), "test preallocated with copy: should own memory");
         check_if_equal(test, test_from_mem, "test construct from mem: equality");
         std::copy(test.begin_all_const(), test.end_all_const(), test_from_mem.begin_all());
@@ -560,7 +537,7 @@ ArrayTests::run_tests()
                          "check row-major order in 2D");
         }
         // Array<2,float> preallocated(t2.get_index_range(), &mem[0], false);
-        Array<2, float> preallocated(t2.get_index_range(), vec_to_shared(mem), false);
+        Array<2, float> preallocated(t2.get_index_range(), vec_to_shared(mem));
         // check(!preallocated.owns_memory_for_data(), "test preallocated without copy: should not own memory");
         check_if_equal(t2, preallocated, "test preallocated: equality");
         std::copy(t2.begin_all_const(), t2.end_all_const(), preallocated.begin_all());
@@ -1050,7 +1027,7 @@ ArrayTests::run_tests()
       std::cerr << "vector creation " << t.value() * 1000 << "ms\n";
       t.start();
       // Array<4,int> a1(range, v.data(), false);
-      Array<4, int> a1(range, vec_to_shared(v), false);
+      Array<4, int> a1(range, vec_to_shared(v));
       t.stop();
       // check(!a1.owns_memory_for_data(), "test preallocated without copy: should not own memory");
       create_duration = t.value();