Transfer iterator result ownership

Generated with the assistance of an AI coding tool.

src/ifcgeom/element.h

@@ -79,7 +79,6 @@ namespace ifcopenshell::geom {
 		std::vector<std::shared_ptr<const ifcopenshell::geom::element>> _parent_storage;
 
 		friend class iterator;
-		virtual std::unique_ptr<element> clone() const { return std::make_unique<element>(*this); }
 		void set_parents(std::vector<std::unique_ptr<ifcopenshell::geom::element>>&& 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<element> clone() const override { return std::make_unique<native_element>(*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<element> clone() const override { return std::make_unique<triangulation_element>(*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<element> clone() const override { return std::make_unique<serialized_element>(*this); }
 	};
 }
 

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<std::mutex> 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<std::mutex> lock(element_ready_mutex_);
 		task_result_iterator_++;
 		native_task_result_iterator_++;
 
@@ -540,6 +548,7 @@ express::base ifcopenshell::geom::iterator::next() {
 			}
 		}
 
+		std::lock_guard<std::mutex> lock(element_ready_mutex_);
 		task_result_iterator_++;
 		native_task_result_iterator_++;
 
@@ -552,7 +561,18 @@ std::unique_ptr<ifcopenshell::geom::element> ifcopenshell::geom::iterator::get()
 {
 	validate_iterator_state();
 
-	auto ret = *task_result_iterator_;
+	std::unique_ptr<ifcopenshell::geom::element> ret;
+	{
+		std::lock_guard<std::mutex> 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<ifcopenshell::geom::settings::UseElementHierarchy>().get()) {
@@ -603,7 +623,7 @@ std::unique_ptr<ifcopenshell::geom::element> ifcopenshell::geom::iterator::get()
 		}
 	}
 
-	return ret->clone();
+	return ret;
 }
 
 std::unique_ptr<ifcopenshell::geom::element> 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<std::mutex> 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_++;
 		}
 	}

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<ifcopenshell::geom::element*> all_processed_elements_;
 		std::list<ifcopenshell::geom::native_element*> all_processed_native_elements_;
 
-		std::list<ifcopenshell::geom::element*>::const_iterator task_result_iterator_;
-		std::list<ifcopenshell::geom::native_element*>::const_iterator native_task_result_iterator_;
+		std::list<ifcopenshell::geom::element*>::iterator task_result_iterator_;
+		std::list<ifcopenshell::geom::native_element*>::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<native_element> get_native()
 		{
 			validate_iterator_state();
-			return std::make_unique<native_element>(**native_task_result_iterator_);
+			std::lock_guard<std::mutex> 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<native_element>(result);
 		}
 
 		std::unique_ptr<element> get_object(int id);

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()

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<ifcopenshell::geom::serialized_element*>($1);