Skip to content

Commit

Permalink
Make default handles ref-counted as well
Browse files Browse the repository at this point in the history
  • Loading branch information
tmadlener committed Nov 21, 2023
1 parent a4a9032 commit 133e974
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 16 deletions.
6 changes: 4 additions & 2 deletions include/podio/utilities/MaybeSharedPtr.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,10 @@ class MaybeSharedPtr {

/// Get a raw pointer to the managed pointer and assume ownership.
T* release() {
// From now on we only need to keep track of the control block
m_ctrlBlock->owned = false;
if (m_ctrlBlock) {
// From now on we only need to keep track of the control block
m_ctrlBlock->owned = false;
}
return m_ptr;
}

Expand Down
2 changes: 1 addition & 1 deletion python/templates/Collection.cc.jinja2
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ void {{ collection_type }}::push_back({{ class.bare_type }} object) {
// This path is only possible if we arrive here from an untracked Mutable object
throw std::invalid_argument("Object needs to be tracked by another collection in order for it to be storable in a subset collection");
}
m_storage.entries.push_back(obj);
m_storage.entries.push_back(obj.release());
}

podio::CollectionWriteBuffers {{ collection_type }}::getBuffers() {
Expand Down
2 changes: 1 addition & 1 deletion python/templates/MutableObject.cc.jinja2
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@

{{ full_type }}::{{ full_type }}(podio::utils::MaybeSharedPtr<{{ class.bare_type }}Obj> obj) : m_obj(obj) {}

{{ full_type }}::operator {{ class.bare_type }}() const { return {{ class.bare_type }}(m_obj.get()); }
{{ full_type }}::operator {{ class.bare_type }}() const { return {{ class.bare_type }}(m_obj); }

{% endwith %}

Expand Down
5 changes: 4 additions & 1 deletion python/templates/Object.cc.jinja2
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#include "podio/utilities/MaybeSharedPtr.h"
{% import "macros/utils.jinja2" as utils %}
{% import "macros/implementations.jinja2" as macros %}
// AUTOMATICALLY GENERATED FILE - DO NOT EDIT
Expand All @@ -18,7 +19,9 @@

{{ macros.common_constructors_destructors(class.bare_type, Members) }}

{{ class.bare_type }}::{{ class.bare_type }}({{ class.bare_type }}Obj* obj) : m_obj(obj) {}
{{ class.bare_type }}::{{ class.bare_type }}(podio::utils::MaybeSharedPtr<{{ class.bare_type }}Obj> obj) : m_obj(obj) {}

{{ class.bare_type }}::{{ class.bare_type }}({{ class.bare_type }}Obj* obj) : m_obj(podio::utils::MaybeSharedPtr<{{ class.bare_type }}Obj>(obj)) {}

{{ class.bare_type }} {{ class.bare_type }}::makeEmpty() {
return {nullptr};
Expand Down
9 changes: 6 additions & 3 deletions python/templates/Object.h.jinja2
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
{{ include }}
{% endfor %}

#include "podio/utilities/MaybeSharedPtr.h"

#include <ostream>
#include <cstddef>

Expand Down Expand Up @@ -49,14 +51,15 @@ public:
{{ utils.if_present(ExtraCode, "declaration") }}
{{ macros.common_object_funcs(class.bare_type) }}

/// disconnect from the {{ class.bare_type }}Obj
void unlink() { m_obj = nullptr; }
/// disconnect from {{ class.bare_type }}Obj instance
void unlink() { m_obj = podio::utils::MaybeSharedPtr<{{ class.bare_type }}Obj>{nullptr}; }

private:
/// constructor from existing {{ class.bare_type }}Obj
explicit {{ class.bare_type}}(podio::utils::MaybeSharedPtr<{{ class.bare_type }}Obj> obj);
{{ class.bare_type}}({{ class.bare_type }}Obj* obj);

{{ class.bare_type }}Obj* m_obj{nullptr};
podio::utils::MaybeSharedPtr<{{ class.bare_type }}Obj> m_obj{nullptr};
};

std::ostream& operator<<(std::ostream& o, const {{ class.bare_type }}& value);
Expand Down
4 changes: 2 additions & 2 deletions python/templates/macros/iterator.jinja2
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% macro iterator_declaration(class, prefix='') %}
{% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %}
{% set ptr_init = 'podio::utils::MaybeSharedPtr<' + class.bare_type +'Obj>{nullptr}' if prefix else 'nullptr' %}
{% set ptr_init = 'podio::utils::MaybeSharedPtr<' + class.bare_type +'Obj>{nullptr}' %}
class {{ iterator_type }} {
public:
{{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object({{ ptr_init }}), m_collection(collection) {}
Expand Down Expand Up @@ -31,7 +31,7 @@ private:

{% macro iterator_definitions(class, prefix='') %}
{% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %}
{% set ptr_type = 'podio::utils::MaybeSharedPtr<' + class.bare_type +'Obj>' if prefix else '' %}
{% set ptr_type = 'podio::utils::MaybeSharedPtr<' + class.bare_type +'Obj>' %}
{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() {
m_object.m_obj = {{ ptr_type }}((*m_collection)[m_index]);
return m_object;
Expand Down
47 changes: 41 additions & 6 deletions tests/unittests/unittest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -149,14 +149,24 @@ TEST_CASE("Component", "[basics]") {
REQUIRE(3 == info.component().data.x);
}

TEST_CASE("makeEmpty", "[basics]") {
auto hit = ExampleHit::makeEmpty();
// Any access to such a handle is a crash
REQUIRE_FALSE(hit.isAvailable());

hit = MutableExampleHit{};
REQUIRE(hit.isAvailable());
REQUIRE(hit.energy() == 0);
}

TEST_CASE("Cyclic", "[basics][relations][memory-management]") {
auto start = MutableExampleForCyclicDependency1();
auto isAvailable = start.ref().isAvailable();
REQUIRE_FALSE(isAvailable);
auto end = MutableExampleForCyclicDependency2();
auto coll1 = ExampleForCyclicDependency1Collection();
auto start = coll1.create();
REQUIRE_FALSE(start.ref().isAvailable());
auto coll2 = ExampleForCyclicDependency2Collection();
auto end = coll2.create();
start.ref(end);
isAvailable = start.ref().isAvailable();
REQUIRE(isAvailable);
REQUIRE(start.ref().isAvailable());
end.ref(start);
REQUIRE(start == end.ref());
auto end_eq = start.ref();
Expand All @@ -165,6 +175,31 @@ TEST_CASE("Cyclic", "[basics][relations][memory-management]") {
REQUIRE(start == start.ref().ref());
}

TEST_CASE("Cyclic w/o collection", "[LEAK-FAIL][basics][relations][memory-management]") {
auto start = MutableExampleForCyclicDependency1{};
REQUIRE_FALSE(start.ref().isAvailable());
auto end = MutableExampleForCyclicDependency2{};
start.ref(end);
REQUIRE(start.ref().isAvailable());
end.ref(start);
REQUIRE(start == end.ref());
auto end_eq = start.ref();
auto start_eq = end_eq.ref();
REQUIRE(start == start_eq);
REQUIRE(start == start.ref().ref());
}

TEST_CASE("Container lifetime", "[basics][memory-management]") {
std::vector<ExampleHit> hits;
{
MutableExampleHit hit;
hit.energy(3.14f);
hits.push_back(hit);
}
auto hit = hits[0];
REQUIRE(hit.energy() == 3.14f);
}

TEST_CASE("Invalid_refs", "[basics][relations]") {
auto store = podio::EventStore();
auto& hits = store.create<ExampleHitCollection>("hits");
Expand Down

0 comments on commit 133e974

Please sign in to comment.