diff --git a/CMakeLists.txt b/CMakeLists.txt index 70fba4405..762a9e40e 100755 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -36,7 +36,7 @@ endif() # ``-DCMAKE_CXX_STANDARD=`` when invoking CMake set(CMAKE_CXX_STANDARD 17 CACHE STRING "") -if(NOT CMAKE_CXX_STANDARD MATCHES "17|20") +if(NOT CMAKE_CXX_STANDARD MATCHES "17|20|23") message(FATAL_ERROR "Unsupported C++ standard: ${CMAKE_CXX_STANDARD}") endif() diff --git a/doc/examples.md b/doc/examples.md index 86a93262f..81ed0de64 100644 --- a/doc/examples.md +++ b/doc/examples.md @@ -65,7 +65,9 @@ If asking for an entry outside bounds, a std::out_of_range exception is thrown. ### Looping through Collections -Looping through collections is supported in two ways. Via iterators: +Looping through collections is supported in several ways. + +Via iterators: ```cpp for(auto i = hits.begin(), end = hits.end(); i != end; ++i) { @@ -73,7 +75,7 @@ Looping through collections is supported in two ways. Via iterators: } ``` -and via direct object access: +Via direct object access: ```cpp for(int i = 0, end = hits.size(), i != end, ++i){ @@ -81,6 +83,36 @@ and via direct object access: } ``` +Via ranged for loops: + +```cpp + for(auto hit: hits) { + std::cout << hit.energy() << std::endl; + } +``` + +Via `std::for_each`: + +```cpp + std::for_each(hits.begin(), hits.end(), [](const auto& hit) { + std::cout << hit.energy() << std::endl; + }); +``` + +Via C++20 ranges and views: + +```cpp + auto is_electron = [](const auto& p){ return p.PDG() == 11; }; + for (auto hit: hits | std::views::filter(is_electron)) { + std::cout << hit.energy() << std::endl; + } +``` + +Note, however, that writing to collections from iterators is not expected to work: +```cpp + std::fill(hits.begin(), hits.end(), Hit()); // bad! iterator lvalues do not modify collection! +``` + ### Support for Notebook-Pattern The `notebook pattern` uses the assumption that it is better to create a small diff --git a/python/templates/Collection.cc.jinja2 b/python/templates/Collection.cc.jinja2 index b76af221b..d4898eeed 100644 --- a/python/templates/Collection.cc.jinja2 +++ b/python/templates/Collection.cc.jinja2 @@ -1,6 +1,6 @@ {% import "macros/utils.jinja2" as utils %} {% import "macros/collections.jinja2" as macros %} -{% from "macros/iterator.jinja2" import iterator_definitions %} +{% from "macros/iterator.jinja2" import iterator_definitions, reverse_iterator_definitions %} // AUTOMATICALLY GENERATED FILE - DO NOT EDIT #include "{{ incfolder }}{{ class.bare_type }}Collection.h" @@ -152,6 +152,10 @@ podio::CollectionBuffers {{ collection_type }}::getBuffers() { {{ iterator_definitions(class, prefix='Mutable' ) }} +{{ reverse_iterator_definitions(class) }} + +{{ reverse_iterator_definitions(class, prefix='Mutable' ) }} + {{ macros.ostream_operator(class, Members, OneToOneRelations, OneToManyRelations, VectorMembers, use_get_syntax, ostream_collection_settings) }} {{ utils.namespace_close(class.namespace) }} diff --git a/python/templates/Collection.h.jinja2 b/python/templates/Collection.h.jinja2 index 50b246700..b89fb8be0 100644 --- a/python/templates/Collection.h.jinja2 +++ b/python/templates/Collection.h.jinja2 @@ -1,6 +1,7 @@ {% import "macros/utils.jinja2" as utils %} {% import "macros/collections.jinja2" as macros %} {% from "macros/iterator.jinja2" import iterator_declaration %} +{% from "macros/iterator.jinja2" import reverse_iterator_declaration %} // AUTOMATICALLY GENERATED FILE - DO NOT EDIT #ifndef {{ package_name.upper() }}_{{ class.bare_type }}Collection_H @@ -26,13 +27,18 @@ #include #include #include +#if __has_include() +# include +#endif {{ utils.namespace_open(class.namespace) }} {{ iterator_declaration(class) }} +{{ reverse_iterator_declaration(class) }} {{ iterator_declaration(class, prefix='Mutable') }} +{{ reverse_iterator_declaration(class, prefix='Mutable') }} /** A Collection is identified by an ID. @@ -41,8 +47,19 @@ class {{ class.bare_type }}Collection : public podio::CollectionBase { public: using const_iterator = {{ class.bare_type }}CollectionIterator; using iterator = {{ class.bare_type }}MutableCollectionIterator; + using const_reverse_iterator = {{ class.bare_type }}CollectionReverseIterator; + using reverse_iterator = {{ class.bare_type }}MutableCollectionReverseIterator; {{ class.bare_type }}Collection(); + #if __cpp_lib_ranges + {{ class.bare_type }}Collection(std::ranges::input_range auto&& range) + requires(std::convertible_to, {{ class.bare_type }}>) + : m_isValid(false), m_isPrepared(false), m_isSubsetColl(false), m_collectionID(0), m_storage() { + for (auto&& item : range) { + push_back(item); + } + }; + #endif // This is a move-only type {{ class.bare_type }}Collection(const {{ class.bare_type}}Collection& ) = delete; {{ class.bare_type }}Collection& operator=(const {{ class.bare_type}}Collection& ) = delete; @@ -120,18 +137,42 @@ public: } // support for the iterator protocol - iterator begin() { + iterator begin() noexcept { return iterator(0, &m_storage.entries); } - const_iterator begin() const { + const_iterator cbegin() const noexcept { return const_iterator(0, &m_storage.entries); } - iterator end() { + const_iterator begin() const noexcept { + return const_iterator(0, &m_storage.entries); + } + reverse_iterator rbegin() noexcept { + return reverse_iterator(m_storage.entries.size() - 1, &m_storage.entries); + } + const_reverse_iterator crbegin() const noexcept { + return const_reverse_iterator(m_storage.entries.size() - 1, &m_storage.entries); + } + const_reverse_iterator rbegin() const noexcept { + return const_reverse_iterator(m_storage.entries.size() - 1, &m_storage.entries); + } + iterator end() noexcept { return iterator(m_storage.entries.size(), &m_storage.entries); } - const_iterator end() const { + const_iterator cend() const noexcept { + return const_iterator(m_storage.entries.size(), &m_storage.entries); + } + const_iterator end() const noexcept { return const_iterator(m_storage.entries.size(), &m_storage.entries); } + reverse_iterator rend() noexcept { + return reverse_iterator(-1, &m_storage.entries); + } + const_reverse_iterator crend() const noexcept { + return const_reverse_iterator(-1, &m_storage.entries); + } + const_reverse_iterator rend() const noexcept { + return const_reverse_iterator(-1, &m_storage.entries); + } {% for member in Members %} template diff --git a/python/templates/MutableObject.h.jinja2 b/python/templates/MutableObject.h.jinja2 index 333c4963d..5fa1fe2ba 100644 --- a/python/templates/MutableObject.h.jinja2 +++ b/python/templates/MutableObject.h.jinja2 @@ -24,6 +24,7 @@ class Mutable{{ class.bare_type }} { friend class {{ class.bare_type }}Collection; friend class {{ class.bare_type }}MutableCollectionIterator; + friend class {{ class.bare_type }}MutableCollectionReverseIterator; friend class {{ class.bare_type }}; public: diff --git a/python/templates/Object.h.jinja2 b/python/templates/Object.h.jinja2 index 2232ffa97..56b839696 100644 --- a/python/templates/Object.h.jinja2 +++ b/python/templates/Object.h.jinja2 @@ -24,6 +24,7 @@ class {{ class.bare_type }} { friend class Mutable{{ class.bare_type }}; friend class {{ class.bare_type }}Collection; friend class {{ class.bare_type }}CollectionIterator; + friend class {{ class.bare_type }}CollectionReverseIterator; public: {{ macros.constructors_destructors(class.bare_type, Members) }} diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index eaba99ba8..a77fe7023 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -2,18 +2,41 @@ {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} class {{ iterator_type }} { public: - {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {} + using difference_type = std::ptrdiff_t; + using value_type = {{ prefix }}{{ class.bare_type }}; + using pointer = {{ prefix }}{{ class.bare_type }}*; + using reference = {{ prefix }}{{ class.bare_type }}&; + using iterator_category = std::input_iterator_tag; - {{ iterator_type }}(const {{ iterator_type }}&) = delete; - {{ iterator_type }}& operator=(const {{ iterator_type }}&) = delete; + {{ iterator_type }}() + : m_index(0), m_object(nullptr), m_collection(nullptr) {}; + {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) + : m_index(index), m_object(nullptr), m_collection(collection) {}; + + {{ iterator_type }}(const {{ iterator_type }}&) = default; + {{ iterator_type }}& operator=(const {{ iterator_type }}&) = default; bool operator!=(const {{ iterator_type}}& x) const { return m_index != x.m_index; // TODO: may not be complete } + bool operator==(const {{ iterator_type}}& x) const { + return !operator!=(x); + } - {{ prefix }}{{ class.bare_type }} operator*(); + {{ prefix }}{{ class.bare_type }} operator*() const; {{ prefix }}{{ class.bare_type }}* operator->(); {{ iterator_type }}& operator++(); + {{ iterator_type }} operator++(int) { + {{ iterator_type }} temp = *this; + ++*this; + return temp; + }; + {{ iterator_type }}& operator--(); + {{ iterator_type }} operator--(int) { + {{ iterator_type }} temp = *this; + --*this; + return temp; + }; private: size_t m_index; @@ -23,12 +46,58 @@ private: {% endwith %} {% endmacro %} +{% macro reverse_iterator_declaration(class, prefix='') %} +{% with iterator_type = class.bare_type + prefix + 'CollectionReverseIterator' %} +class {{ iterator_type }} { +public: + using difference_type = std::ptrdiff_t; + using value_type = {{prefix}}{{ class.bare_type }}; + using pointer = {{prefix}}{{ class.bare_type }}*; + using reference = {{prefix}}{{ class.bare_type }}&; + using iterator_category = std::input_iterator_tag; + + {{ iterator_type }}() + : m_index(0), m_object(nullptr), m_collection(nullptr) {}; + {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) + : m_index(index), m_object(nullptr), m_collection(collection) {}; + + {{ iterator_type }}(const {{ iterator_type }}&) = default; + {{ iterator_type }}& operator=(const {{ iterator_type }}&) = default; + + bool operator!=(const {{ iterator_type }}& x) const { + return m_index != x.m_index; // TODO: may not be complete + } + bool operator==(const {{ iterator_type }}& x) const { + return !operator!=(x); + } + + {{ prefix }}{{ class.bare_type }} operator*() const; + {{ prefix }}{{ class.bare_type }}* operator->(); + {{ iterator_type }}& operator++(); + {{ iterator_type }} operator++(int) { + {{ iterator_type }} temp = *this; + ++*this; + return temp; + }; + {{ iterator_type }}& operator--(); + {{ iterator_type }} operator--(int) { + {{ iterator_type }} temp = *this; + --*this; + return temp; + }; + +private: + size_t m_index; + {{ prefix }}{{ class.bare_type }} m_object; + const {{ class.bare_type }}ObjPointerContainer* m_collection; +}; +{% endwith %} +{% endmacro %} {% macro iterator_definitions(class, prefix='') %} {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} -{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() { - m_object.m_obj = (*m_collection)[m_index]; - return m_object; +{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const { + return {(*m_collection)[m_index]}; } {{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() { @@ -41,5 +110,32 @@ private: return *this; } +{{ iterator_type }}& {{ iterator_type }}::operator--() { + --m_index; + return *this; +} +{% endwith %} +{% endmacro %} + +{% macro reverse_iterator_definitions(class, prefix='') %} +{% with iterator_type = class.bare_type + prefix + 'CollectionReverseIterator' %} +{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const { + return {(*m_collection)[m_index]}; +} + +{{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() { + m_object.m_obj = (*m_collection)[m_index]; + return &m_object; +} + +{{ iterator_type }}& {{ iterator_type }}::operator++() { + --m_index; + return *this; +} + +{{ iterator_type }}& {{ iterator_type }}::operator--() { + ++m_index; + return *this; +} {% endwith %} {% endmacro %} diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 635ea91f3..a6a2954a3 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -1,10 +1,19 @@ // STL +#include #include #include #include #include #include #include +#ifdef __has_include + #if __has_include() + #include + #endif + #if __has_include() + #include + #endif +#endif #include "catch2/catch_test_macros.hpp" @@ -494,6 +503,186 @@ TEST_CASE("const correct iterators on collections", "[const-correctness]") { // objects } +TEST_CASE("Iterator dereferencing", "[!shouldfail][iterators]") { + // a collection to play with + auto collection = ExampleHitCollection(); + + // NOTE: the following unit tests will fail because even the mutable iterator + // does not return an lvalue on dereference that writes to the collection: i.e. + // for (auto i = collection.begin(); i != collection.end(); i++) { + // *i = MutableExampleHit(0x42ULL, 0., 0., 0., 0.); + // } + // This does not write into the collection, but just to a temporary copy. + + // fill from mutable + collection.clear(); + collection.create(); + auto mutable_hit = MutableExampleHit(0x42ULL, 0., 0., 0., 0.); + std::fill(collection.begin(), collection.end(), mutable_hit); + REQUIRE(collection.begin()->cellID() == 0x42ULL); +} + +TEST_CASE("Iterator concepts", "[iterator-concepts][iterators]") { + // a collection to play with + auto collection = ExampleHitCollection(); + + // input iterator traits + STATIC_REQUIRE( + std::is_same_v::iterator_category, std::input_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, + std::input_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, + std::input_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, + std::input_iterator_tag>); + + STATIC_REQUIRE_FALSE(std::is_same_v::iterator_category, + std::forward_iterator_tag>); + STATIC_REQUIRE_FALSE(std::is_same_v::iterator_category, + std::forward_iterator_tag>); + STATIC_REQUIRE_FALSE(std::is_same_v::iterator_category, + std::forward_iterator_tag>); + STATIC_REQUIRE_FALSE( + std::is_same_v::iterator_category, + std::forward_iterator_tag>); + +#if __cpp_concepts + // input iterator concepts (C++20) + STATIC_REQUIRE(std::input_iterator); + STATIC_REQUIRE(std::input_iterator); + STATIC_REQUIRE(std::input_iterator); + STATIC_REQUIRE(std::input_iterator); + // but not foward iterator concept + STATIC_REQUIRE_FALSE(std::forward_iterator); + STATIC_REQUIRE_FALSE(std::forward_iterator); + STATIC_REQUIRE_FALSE(std::forward_iterator); + STATIC_REQUIRE_FALSE(std::forward_iterator); +#endif + + // iterator loops + auto cellID = 0; + collection.clear(); + collection.create(); + collection.create(0x42ULL, 0., 0., 0., 0.); + // forward loop with post-increment + for (auto c = collection.begin(); c != collection.end(); c++) { + STATIC_REQUIRE(std::is_same_v); + cellID = c->cellID(); + } + REQUIRE(cellID == 0x42ULL); + // reverse const loop with pre-increment + for (auto c = collection.crbegin(); c != collection.crend(); ++c) { + STATIC_REQUIRE(std::is_same_v); + cellID = c->cellID(); + } + REQUIRE(cellID == 0); + // std::for_each loop with reference capture + std::for_each(collection.begin(), collection.end(), [&cellID](const auto& c) { + STATIC_REQUIRE(std::is_same_v); + cellID = c.cellID(); + }); + REQUIRE(cellID == 0x42ULL); + + // find in collection + collection.clear(); + collection.create(); + collection.create(0x42ULL, 1., 1., 1., 1.); + // forward find with const iterators + auto found_const = + std::find_if(collection.cbegin(), collection.cend(), [](const auto& h) { return h.cellID() == 0x42ULL; }); + STATIC_REQUIRE(std::is_same_v); + REQUIRE(found_const != collection.cend()); + REQUIRE(found_const->cellID() == 0x42ULL); + // forward find with mutable iterators + auto found = std::find_if(collection.begin(), collection.end(), [](const auto& h) { return h.cellID() == 0x42ULL; }); + STATIC_REQUIRE(std::is_same_v); + REQUIRE(found != collection.end()); + REQUIRE(found->cellID() == 0x42ULL); + // reverse find with mutable iterators + auto found_reverse = + std::find_if(collection.rbegin(), collection.rend(), [](const auto& h) { return h.cellID() == 0x42ULL; }); + STATIC_REQUIRE(std::is_same_v); + REQUIRE(found_reverse != collection.rend()); + REQUIRE(found_reverse->cellID() == 0x42ULL); + // forward find that fails + auto not_found = + std::find_if(collection.begin(), collection.end(), [](const auto& h) { return h.cellID() == 0x1ULL; }); + REQUIRE(not_found == collection.end()); + // reverse find that fails + auto not_found_reverse = + std::find_if(collection.rbegin(), collection.rend(), [](const auto& h) { return h.cellID() == 0x1ULL; }); + REQUIRE(not_found_reverse == collection.rend()); +} + +#ifdef __cpp_lib_ranges +TEST_CASE("Ranges (C++20)", "[ranges][iterators]") { + // a collection to play with + auto collection = ExampleHitCollection(); + + STATIC_REQUIRE(std::ranges::input_range); + + // ranged views (C++20) + auto is_non_zero = [](const auto& p) { return p.cellID() > 0; }; + auto half_energy = [](const auto& p) { + auto q = p.clone(); + q.energy(q.energy() / 2); + return q; + }; + collection.clear(); + collection.create(); + collection.create(0x42ULL, 1., 1., 1., 1.); + // pipe syntax + int count_pipe_syntax = 0; + for (auto c : collection | std::views::filter(is_non_zero)) { + STATIC_REQUIRE(std::is_same_v); + ++count_pipe_syntax; + } + REQUIRE(count_pipe_syntax == 1); + // composing syntax + auto count_composing = 0; + for (auto c : std::views::filter(collection, is_non_zero)) { + STATIC_REQUIRE(std::is_same_v); + ++count_composing; + } + REQUIRE(count_composing == 1); + // take_while + auto count_take_while = 0; + for (auto c : collection | std::views::take_while(is_non_zero)) { + STATIC_REQUIRE(std::is_same_v); + ++count_take_while; + } + REQUIRE(count_take_while == 0); + // ref_view + const std::ranges::ref_view collection_ref_view{collection}; + for (auto c : collection_ref_view) { + STATIC_REQUIRE(std::is_same_v); + } + // for_each with pipe syntax + float energy = 0; + auto add_to_energy = [&e = energy](const auto& p) { e += p.energy(); }; + std::ranges::for_each(collection | std::views::filter(is_non_zero) | std::views::transform(half_energy), + add_to_energy); + REQUIRE(energy > 0.49); + REQUIRE(energy < 0.51); + #ifdef __cpp_lib_ranges_zip + // zip range view over two different collections (C++23) + auto collection1 = ExampleHitCollection(); + auto hit1 = collection1.create(); + auto collection2 = ExampleClusterCollection(); + auto cluster1 = collection2.create(); + REQUIRE(collection1.begin().cellID() == 0); + for (auto c: std::views::zip(collection1, collection2) { + STATIC_REQUIRE(std::is_same_v(c)), MutableExampleHit>); + STATIC_REQUIRE(std::is_same_v(c)), MutableExampleCluster>); + // set all cellIDs in hit collection to 1 + std::get<0>(c).cellID(1); + } + // views are references + REQUIRE(collection1.begin().cellID() == 1); + #endif +} +#endif + TEST_CASE("Subset collection basics", "[subset-colls]") { auto clusterRefs = ExampleClusterCollection(); clusterRefs.setSubsetCollection();