diff --git a/src/ifcgeom/element.h b/src/ifcgeom/element.h index 838c75cbc9..5fef276d99 100644 --- a/src/ifcgeom/element.h +++ b/src/ifcgeom/element.h @@ -79,7 +79,6 @@ namespace ifcopenshell::geom { std::vector> _parent_storage; friend class iterator; - virtual std::unique_ptr clone() const { return std::make_unique(*this); } void set_parents(std::vector>&& newparents) { _parents.clear(); _parent_storage.clear(); @@ -177,7 +176,6 @@ namespace ifcopenshell::geom { native_element(const native_element& other) = default; private: native_element& operator=(const native_element& other); - std::unique_ptr clone() const override { return std::make_unique(*this); } }; class triangulation_element : public element { @@ -197,7 +195,6 @@ namespace ifcopenshell::geom { triangulation_element(const triangulation_element& other) = default; private: triangulation_element& operator=(const triangulation_element& other); - std::unique_ptr clone() const override { return std::make_unique(*this); } }; class serialized_element : public element { @@ -212,7 +209,6 @@ namespace ifcopenshell::geom { serialized_element(const serialized_element& other) = default; private: serialized_element& operator=(const serialized_element& other); - std::unique_ptr clone() const override { return std::make_unique(*this); } }; } diff --git a/src/ifcgeom/iterator.cpp b/src/ifcgeom/iterator.cpp index 3b978d3f7a..7d47288562 100644 --- a/src/ifcgeom/iterator.cpp +++ b/src/ifcgeom/iterator.cpp @@ -509,10 +509,17 @@ express::base ifcopenshell::geom::iterator::next() { using std::chrono::high_resolution_clock; validate_iterator_state(); - if (*native_task_result_iterator_ != *task_result_iterator_) { - delete* native_task_result_iterator_; + { + std::lock_guard lock(element_ready_mutex_); + auto* element = *task_result_iterator_; + auto* native_element = *native_task_result_iterator_; + *task_result_iterator_ = nullptr; + *native_task_result_iterator_ = nullptr; + if (native_element != element) { + delete native_element; + } + delete element; } - delete* task_result_iterator_; if (num_threads_ != 1) { if (!wait_for_element()) { @@ -523,6 +530,7 @@ express::base ifcopenshell::geom::iterator::next() { return express::base{}; } + std::lock_guard lock(element_ready_mutex_); task_result_iterator_++; native_task_result_iterator_++; @@ -540,6 +548,7 @@ express::base ifcopenshell::geom::iterator::next() { } } + std::lock_guard lock(element_ready_mutex_); task_result_iterator_++; native_task_result_iterator_++; @@ -552,7 +561,18 @@ std::unique_ptr ifcopenshell::geom::iterator::get() { validate_iterator_state(); - auto ret = *task_result_iterator_; + std::unique_ptr ret; + { + std::lock_guard lock(element_ready_mutex_); + ret.reset(*task_result_iterator_); + if (!ret) { + throw std::runtime_error("current element has already been retrieved"); + } + *task_result_iterator_ = nullptr; + if (*native_task_result_iterator_ == ret.get()) { + *native_task_result_iterator_ = nullptr; + } + } // If we want to organize the element considering their hierarchy if (settings_.get().get()) { @@ -603,7 +623,7 @@ std::unique_ptr ifcopenshell::geom::iterator::get() } } - return ret->clone(); + return ret; } std::unique_ptr ifcopenshell::geom::iterator::get_object(int id) { @@ -842,11 +862,15 @@ ifcopenshell::geom::iterator::~iterator() { } if (task_result_ptr_initialized) { - while (task_result_iterator_ != --all_processed_elements_.end()) { - if (*native_task_result_iterator_ != *task_result_iterator_) { - delete* native_task_result_iterator_; + std::lock_guard lock(element_ready_mutex_); + while (task_result_iterator_ != all_processed_elements_.end()) { + auto* element = *task_result_iterator_; + auto* native_element = *native_task_result_iterator_; + if (native_element != element) { + delete native_element; } - delete* task_result_iterator_++; + delete element; + task_result_iterator_++; native_task_result_iterator_++; } } diff --git a/src/ifcgeom/iterator.h b/src/ifcgeom/iterator.h index fe46a3559f..45d8987d1e 100644 --- a/src/ifcgeom/iterator.h +++ b/src/ifcgeom/iterator.h @@ -44,7 +44,7 @@ * at least a single representation will process successfully * * * * ifcopenshell::geom::iterator::get() * - * returns an owned copy of the current ifcopenshell::geom::element * + * transfers ownership of the current ifcopenshell::geom::element * * * * ifcopenshell::geom::iterator::next() * * returns true iff a following entity is available for a successive call to * @@ -115,8 +115,8 @@ namespace ifcopenshell::geom { std::list all_processed_elements_; std::list all_processed_native_elements_; - std::list::const_iterator task_result_iterator_; - std::list::const_iterator native_task_result_iterator_; + std::list::iterator task_result_iterator_; + std::list::iterator native_task_result_iterator_; std::mutex element_ready_mutex_; bool task_result_ptr_initialized = false; @@ -297,7 +297,16 @@ namespace ifcopenshell::geom { std::unique_ptr get_native() { validate_iterator_state(); - return std::make_unique(**native_task_result_iterator_); + std::lock_guard lock(element_ready_mutex_); + auto* result = *native_task_result_iterator_; + if (!result) { + throw std::runtime_error("current native element has already been retrieved"); + } + *native_task_result_iterator_ = nullptr; + if (*task_result_iterator_ == result) { + *task_result_iterator_ = nullptr; + } + return std::unique_ptr(result); } std::unique_ptr get_object(int id); diff --git a/src/ifcopenshell-python/test/test_create_shape.py b/src/ifcopenshell-python/test/test_create_shape.py index 65c6edb501..218b95daa2 100644 --- a/src/ifcopenshell-python/test/test_create_shape.py +++ b/src/ifcopenshell-python/test/test_create_shape.py @@ -203,6 +203,36 @@ def test_iterator(): assert iterator.initialize() +@pytest.mark.parametrize("num_threads", [1, 2]) +def test_iterator_get_transfers_ownership(num_threads): + settings = ifcopenshell.geom.settings() + iterator = ifcopenshell.geom.iterator(settings, fn, num_threads) + assert iterator.initialize() + + element = iterator.get() + element_id = element.id + with pytest.raises(RuntimeError, match="already been retrieved"): + iterator.get() + + iterator.next() + assert element.id == element_id + + +@pytest.mark.parametrize("num_threads", [1, 2]) +def test_iterator_get_native_transfers_ownership(num_threads): + settings = ifcopenshell.geom.settings() + iterator = ifcopenshell.geom.iterator(settings, fn, num_threads) + assert iterator.initialize() + + element = iterator.get_native() + element_id = element.id + with pytest.raises(RuntimeError, match="already been retrieved"): + iterator.get_native() + + iterator.next() + assert element.id == element_id + + def test_logging(): assert ifcopenshell.logger logger = ifcopenshell.logger() diff --git a/src/ifcwrap/IfcGeomWrapper.i b/src/ifcwrap/IfcGeomWrapper.i index f220266497..1619412d66 100644 --- a/src/ifcwrap/IfcGeomWrapper.i +++ b/src/ifcwrap/IfcGeomWrapper.i @@ -68,7 +68,7 @@ } // Use RTTI to return the most specialized element proxy. The ownership flag is -// supplied by the wrapped function, so iterator copies can transfer ownership +// supplied by the wrapped function, so iterator results can transfer ownership // while borrowed serializer results remain non-owning. %typemap(out) ifcopenshell::geom::element* { ifcopenshell::geom::serialized_element* serialized_elem = dynamic_cast($1);