From dfcac71d467a99f545080b87684e1c6819680861 Mon Sep 17 00:00:00 2001 From: Kris Thielemans Date: Mon, 12 Feb 2024 02:10:02 +0000 Subject: [PATCH] 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 | 32 +++++------ src/include/stir/Array.inl | 26 ++++----- src/include/stir/NumericVectorWithOffset.h | 9 +--- src/include/stir/NumericVectorWithOffset.inl | 15 ------ src/include/stir/VectorWithOffset.h | 57 +++++++++++++++----- src/include/stir/VectorWithOffset.inl | 37 ++++++++++--- src/test/test_Array.cxx | 37 +++---------- 7 files changed, 109 insertions(+), 104 deletions(-) diff --git a/src/include/stir/Array.h b/src/include/stir/Array.h index 83db988854..83ca9e9f26 100644 --- a/src/include/stir/Array.h +++ b/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, e //! Construct an Array of given range of indices, elements are initialised to 0 inline explicit Array(const IndexRange&); - //! 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& range, shared_ptr data_sptr, bool copy_data); + inline Array(const IndexRange& range, shared_ptr 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 //! 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 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 data_sptr, bool copy_data); + inline Array(const IndexRange<1>& range, const elemT* const data_ptr); //! constructor from basetype inline Array(const NumericVectorWithOffset& il); diff --git a/src/include/stir/Array.inl b/src/include/stir/Array.inl index 52e664e970..aa3c11dce9 100644 --- a/src/include/stir/Array.inl +++ b/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::Array(const IndexRange& range) } template -Array::Array(const IndexRange& range, shared_ptr data_sptr, bool copy_data) +Array::Array(const IndexRange& range, shared_ptr data_sptr) { - if (copy_data) - { - this->_allocated_full_data_ptr = std::shared_ptr(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 -Array<1, elemT>::Array(const IndexRange<1>& range, shared_ptr 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 data_sptr) + : base_type(range.get_min_index(), range.get_max_index(), data_sptr) +{} + +template +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 Array<1, elemT>::Array(const base_type& il) diff --git a/src/include/stir/NumericVectorWithOffset.h b/src/include/stir/NumericVectorWithOffset.h index e074f80418..fae4620f0c 100644 --- a/src/include/stir/NumericVectorWithOffset.h +++ b/src/include/stir/NumericVectorWithOffset.h @@ -50,14 +50,7 @@ class NumericVectorWithOffset : public VectorWithOffset typedef VectorWithOffset 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); diff --git a/src/include/stir/NumericVectorWithOffset.inl b/src/include/stir/NumericVectorWithOffset.inl index 4b2b793af1..dba3a7a970 100644 --- a/src/include/stir/NumericVectorWithOffset.inl +++ b/src/include/stir/NumericVectorWithOffset.inl @@ -27,21 +27,6 @@ START_NAMESPACE_STIR -template -inline NumericVectorWithOffset::NumericVectorWithOffset() - : base_type() -{} - -template -inline NumericVectorWithOffset::NumericVectorWithOffset(const int hsz) - : base_type(hsz) -{} - -template -inline NumericVectorWithOffset::NumericVectorWithOffset(const int min_index, const int max_index) - : base_type(min_index, max_index) -{} - template NumericVectorWithOffset::NumericVectorWithOffset(const VectorWithOffset& t) : base_type(t) diff --git a/src/include/stir/VectorWithOffset.h b/src/include/stir/VectorWithOffset.h index 7c29baf70d..78043ea10b 100644 --- a/src/include/stir/VectorWithOffset.h +++ b/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 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 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 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 allocated_memory_sptr; + //! Default member settings for all constructors inline void init(); diff --git a/src/include/stir/VectorWithOffset.inl b/src/include/stir/VectorWithOffset.inl index 963bb44528..ea003af390 100644 --- a/src/include/stir/VectorWithOffset.inl +++ b/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,17 +271,13 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index) this->check_state(); } -template -VectorWithOffset::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 VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, T* const data_ptr, T* const end_of_data_ptr) : length(static_cast(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; @@ -289,6 +285,33 @@ VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, this->check_state(); } +template +VectorWithOffset::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 +VectorWithOffset::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(data_ptr), /* copy_data = */ true); +} + +template +VectorWithOffset::VectorWithOffset(const int sz, const T* const data_ptr) + : VectorWithOffset(0, sz - 1, data_ptr) +{} + +template +VectorWithOffset::VectorWithOffset(const int min_index, const int max_index, shared_ptr data_sptr) +{ + this->allocated_memory_sptr = data_sptr; + this->init(min_index, max_index, data_sptr.get(), /* copy_data = */ false); +} + template VectorWithOffset::VectorWithOffset(VectorWithOffset&& other) noexcept : VectorWithOffset() diff --git a/src/test/test_Array.cxx b/src/test/test_Array.cxx index 2f655ffd95..8371e4825c 100644 --- a/src/test/test_Array.cxx +++ b/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 -class Deleter -{ -public: - Deleter(std::vector& v) - : v(v) - {} - void operator()(T* vptr) - { - info("in deleter"); - if (vptr != v.data()) - error("deleter error"); - //~v; - } - -private: - std::vector& v; -}; - +// helper function to create a shared_ptr that doesn't delete the data (as it's still owned by the vector) template shared_ptr vec_to_shared(std::vector& v) { - shared_ptr sptr(v.data(), Deleter(v)); + shared_ptr sptr(v.data(), [](auto) {}); return sptr; } @@ -380,8 +362,7 @@ ArrayTests::run_tests() { std::vector 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 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 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 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(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();