From bc4d5ec94b3b84a0fdf87b44cad45d7bb552f380 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Sat, 14 Dec 2024 16:07:23 +0100 Subject: [PATCH 01/14] add note and support for `std::forward_iterator` --- doc/collections_as_container.md | 14 ++++--- python/templates/macros/iterator.jinja2 | 4 +- tests/unittests/std_interoperability.cpp | 48 ++++++++++++++++++------ 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/doc/collections_as_container.md b/doc/collections_as_container.md index 860d21b8a..b936309a6 100644 --- a/doc/collections_as_container.md +++ b/doc/collections_as_container.md @@ -88,11 +88,15 @@ In the following tables a convention from `Collection` is used: `iterator` stand | `std::input_or_output_iterator` | ✔️ yes | ✔️ yes | | `std::input_iterator` | ✔️ yes | ✔️ yes | | `std::output_iterator` | ❌ no | ❌ no | -| `std::forward_iterator` | ❌ no | ❌ no | +| `std::forward_iterator` | ✔️ yes (see note below) | ✔️ yes (see note below) | | `std::bidirectional_iterator` | ❌ no | ❌ no | | `std::random_access_iterator` | ❌ no | ❌ no | | `std::contiguous_iterator` | ❌ no | ❌ no | +> [!NOTE] +>The collections' iterators fulfil the `std::forward_iterator` except that the pointers obtained with `->` remain valid only as long as the iterator is valid instead of as long as the range remain valid. In practice this means a `ptr` obtained with `auto* ptr = it.operator->();` is valid only as long as `it` is valid. +>The values obtained immediately through `->` (for instance `auto& e = it->energy();`) behaves as expected for `std::forward_iterator` as their validity is tied to the validity of a collection. + ### LegacyIterator | Requirement | Fulfilled by `iterator`/`const_iterator`? | Comment | @@ -180,7 +184,7 @@ In addition to the *LegacyForwardIterator* the C++ standard specifies also the * | `std::ranges::sized_range` | ✔️ yes | | `std::ranges::input_range` | ✔️ yes | | `std::ranges::output_range` | ❌ no | -| `std::ranges::forward_range` | ❌ no | +| `std::ranges::forward_range` | ✔️ yes | | `std::ranges::bidirectional_range` | ❌ no | | `std::ranges::random_access_range` | ❌ no | | `std::ranges::contiguous_range` | ❌ no | @@ -207,16 +211,16 @@ std::sort(std::begin(collection), std::end(collection)); // requires RandomAcces The arguments of standard range algorithms are checked at compile time and must fulfil certain iterator concepts, such as `std::input_iterator` or `std::ranges::input_range`. -The iterators of PODIO collections model the `std::input_iterator` concept, so range algorithms that require this iterator type will work correctly with PODIO iterators. If an algorithm compiles, it is guaranteed to work as expected. +The iterators of PODIO collections model the `std::forward_iterator` concept, so range algorithms that require this iterator type will work correctly with PODIO iterators. If an algorithm compiles, it is guaranteed to work as expected. In particular, the PODIO collections' iterators do not fulfil the `std::output_iterator` concept, and as a result, mutating algorithms relying on this iterator type will not compile. -Similarly the collections themselves model the `std::input_range` concept and can be used in the range algorithms that require that concept. The algorithms requiring unsupported range concept, such as `std::output_range`, won't compile. +Similarly the collections themselves model the `std::forward_range` concept and can be used in the range algorithms that require that concept. The algorithms requiring unsupported range concept, such as `std::output_range`, won't compile. For example: ```c++ std::ranges::find_if(collection, predicate ); // requires input_range -> OK -std::ranges::adjacent_find(collection, predicate ); // requires forward_range -> won't compile +std::ranges::adjacent_find(collection, predicate ); // requires forward_range -> OK std::ranges::fill(collection, value ); // requires output_range -> won't compile std::ranges::sort(collection); // requires random_access_range and sortable -> won't compile ``` diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index d3b512f94..114fbc9f2 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -8,7 +8,9 @@ public: using reference = {{ prefix }}{{ class.bare_type }}; using pointer = {{ prefix }}{{ class.bare_type }}*; using iterator_category = std::input_iterator_tag; - using iterator_concept = std::input_iterator_tag; + // `std::forward_iterator` is supported except that the pointers obtained with `operator->()` + // remain valid as long as the iterator is valid, not as long as the range is valid. + using iterator_concept = std::forward_iterator_tag; {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object({{ ptr_init }}), m_collection(collection) {} {{ iterator_type }}() = default; diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index 591e92f9c..f5c09f8a6 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -497,6 +497,35 @@ TEST_CASE("Collection and iterator concepts", "[collection][container][iterator] // const_iterator STATIC_REQUIRE(std::input_iterator); } + + SECTION("forward_iterator") { + // iterator + STATIC_REQUIRE(std::forward_iterator); + { + REQUIRE(iterator{} == iterator{}); + auto coll = CollectionType(); + coll.create(); + auto i = coll.begin(); + auto j = coll.begin(); + REQUIRE(i == j); + REQUIRE(++i == ++j); + i = coll.begin(); + REQUIRE(((void)[](auto x) { ++x; }(i), *i) == *i); + } + // const_iterator + STATIC_REQUIRE(std::forward_iterator); + { + REQUIRE(const_iterator{} == const_iterator{}); + auto coll = CollectionType(); + coll.create(); + auto i = coll.cbegin(); + auto j = coll.cbegin(); + REQUIRE(i == j); + REQUIRE(++i == ++j); + i = coll.cbegin(); + REQUIRE(((void)[](auto x) { ++x; }(i), *i) == *i); + } + } } TEST_CASE("Collection and unsupported iterator concepts", "[collection][container][iterator][std]") { @@ -510,9 +539,6 @@ TEST_CASE("Collection and unsupported iterator concepts", "[collection][containe DOCUMENTED_STATIC_FAILURE(std::output_iterator); DOCUMENTED_STATIC_FAILURE(std::output_iterator); DOCUMENTED_STATIC_FAILURE(std::output_iterator); - // std::forward_iterator - DOCUMENTED_STATIC_FAILURE(std::forward_iterator); - DOCUMENTED_STATIC_FAILURE(std::forward_iterator); // std::bidirectional_iterator DOCUMENTED_STATIC_FAILURE(std::bidirectional_iterator); DOCUMENTED_STATIC_FAILURE(std::bidirectional_iterator); @@ -1087,7 +1113,7 @@ TEST_CASE("Collection as range", "[collection][ranges][std]") { DOCUMENTED_STATIC_FAILURE(std::ranges::output_range); DOCUMENTED_STATIC_FAILURE(std::ranges::output_range); // std::range::forward_range - DOCUMENTED_STATIC_FAILURE(std::ranges::forward_range); + STATIC_REQUIRE(std::ranges::forward_range); // std::range::bidirectional_range DOCUMENTED_STATIC_FAILURE(std::ranges::bidirectional_range); // std::range::random_access_range @@ -1156,13 +1182,14 @@ TEST_CASE("Collection and std ranges algorithms", "[collection][ranges][std]") { REQUIRE(subcoll.size() == 2); REQUIRE(subcoll[0].cellID() == 5); REQUIRE(subcoll[1].cellID() == 3); -} -// helper concept for unsupported algorithm compilation test -template -concept is_range_adjacent_findable = requires(T coll) { - std::ranges::adjacent_find(coll, [](const auto& a, const auto& b) { return a.cellID() == b.cellID(); }); -}; + auto adjacent_it = + std::ranges::adjacent_find(coll, [](const auto& a, const auto& b) { return a.cellID() == b.cellID(); }); + REQUIRE(adjacent_it != std::end(coll)); + auto target = std::begin(coll); + std::ranges::advance(target, 2); + REQUIRE(adjacent_it == target); +} // helper concept for unsupported algorithm compilation test template @@ -1175,7 +1202,6 @@ concept is_range_fillable = requires(T coll) { std::ranges::fill(coll, typename TEST_CASE("Collection and unsupported std ranges algorithms", "[collection][ranges][std]") { // check that algorithms requiring unsupported iterator concepts won't compile - DOCUMENTED_STATIC_FAILURE(is_range_adjacent_findable); DOCUMENTED_STATIC_FAILURE(is_range_sortable); DOCUMENTED_STATIC_FAILURE(is_range_fillable); } From e9638fecb6e6098e9f23b56030cdbbb73ae98c51 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Sat, 14 Dec 2024 17:56:41 +0100 Subject: [PATCH 02/14] add support for `std::bidirectional_iterator` --- doc/collections_as_container.md | 11 +-- python/templates/macros/iterator.jinja2 | 15 +++- tests/unittests/std_interoperability.cpp | 109 +++++++++++++++++++++-- 3 files changed, 121 insertions(+), 14 deletions(-) diff --git a/doc/collections_as_container.md b/doc/collections_as_container.md index b936309a6..0bb6a495c 100644 --- a/doc/collections_as_container.md +++ b/doc/collections_as_container.md @@ -89,7 +89,7 @@ In the following tables a convention from `Collection` is used: `iterator` stand | `std::input_iterator` | ✔️ yes | ✔️ yes | | `std::output_iterator` | ❌ no | ❌ no | | `std::forward_iterator` | ✔️ yes (see note below) | ✔️ yes (see note below) | -| `std::bidirectional_iterator` | ❌ no | ❌ no | +| `std::bidirectional_iterator` | ✔️ yes | ✔️ yes | | `std::random_access_iterator` | ❌ no | ❌ no | | `std::contiguous_iterator` | ❌ no | ❌ no | @@ -166,7 +166,7 @@ In addition to the *LegacyForwardIterator* the C++ standard specifies also the * | Adaptor | Compatible with Collection? | Comment | |---------|-----------------------------|---------| -| `std::reverse_iterator` | ❌ no | `iterator` and `const_iterator` not *LegacyBidirectionalIterator* or `std::bidirectional_iterator` | +| `std::reverse_iterator` | ❗ attention | `operator->` not defined as `iterator`'s and `const_iterator`'s `operator->` are non-`const` | | `std::back_insert_iterator` | ❗ attention | Compatible only with SubsetCollections, otherwise throws `std::invalid_argument` | | `std::front_insert_iterator` | ❌ no | `push_front` not defined | | `std::insert_iterator` | ❌ no | `insert` not defined | @@ -185,7 +185,7 @@ In addition to the *LegacyForwardIterator* the C++ standard specifies also the * | `std::ranges::input_range` | ✔️ yes | | `std::ranges::output_range` | ❌ no | | `std::ranges::forward_range` | ✔️ yes | -| `std::ranges::bidirectional_range` | ❌ no | +| `std::ranges::bidirectional_range` | ✔️ yes | | `std::ranges::random_access_range` | ❌ no | | `std::ranges::contiguous_range` | ❌ no | | `std::ranges::common_range` | ✔️ yes | @@ -211,16 +211,17 @@ std::sort(std::begin(collection), std::end(collection)); // requires RandomAcces The arguments of standard range algorithms are checked at compile time and must fulfil certain iterator concepts, such as `std::input_iterator` or `std::ranges::input_range`. -The iterators of PODIO collections model the `std::forward_iterator` concept, so range algorithms that require this iterator type will work correctly with PODIO iterators. If an algorithm compiles, it is guaranteed to work as expected. +The iterators of PODIO collections model the `std::bidirectional_iterator` concept, so range algorithms that require this iterator type will work correctly with PODIO iterators. If an algorithm compiles, it is guaranteed to work as expected. In particular, the PODIO collections' iterators do not fulfil the `std::output_iterator` concept, and as a result, mutating algorithms relying on this iterator type will not compile. -Similarly the collections themselves model the `std::forward_range` concept and can be used in the range algorithms that require that concept. The algorithms requiring unsupported range concept, such as `std::output_range`, won't compile. +Similarly the collections themselves model the `std::bidirectional_range` concept and can be used in the range algorithms that require that concept. The algorithms requiring unsupported range concept, such as `std::output_range`, won't compile. For example: ```c++ std::ranges::find_if(collection, predicate ); // requires input_range -> OK std::ranges::adjacent_find(collection, predicate ); // requires forward_range -> OK +std::ranges::views::reverse(collection); //requires bidirectional_range -> OK std::ranges::fill(collection, value ); // requires output_range -> won't compile std::ranges::sort(collection); // requires random_access_range and sortable -> won't compile ``` diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index 114fbc9f2..803c475fe 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -10,7 +10,7 @@ public: using iterator_category = std::input_iterator_tag; // `std::forward_iterator` is supported except that the pointers obtained with `operator->()` // remain valid as long as the iterator is valid, not as long as the range is valid. - using iterator_concept = std::forward_iterator_tag; + using iterator_concept = std::bidirectional_iterator_tag; {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object({{ ptr_init }}), m_collection(collection) {} {{ iterator_type }}() = default; @@ -33,6 +33,8 @@ public: pointer operator->(); {{ iterator_type }}& operator++(); {{ iterator_type }} operator++(int); + {{ iterator_type }}& operator--(); + {{ iterator_type }} operator--(int); private: size_t m_index{0}; @@ -66,5 +68,16 @@ private: return copy; } +{{ iterator_type }}& {{ iterator_type }}::operator--() { + --m_index; + return *this; +} + +{{ iterator_type }} {{ iterator_type }}::operator--(int) { + auto copy = *this; + --m_index; + return copy; +} + {% endwith %} {% endmacro %} diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index f5c09f8a6..93960a4d5 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -526,6 +526,51 @@ TEST_CASE("Collection and iterator concepts", "[collection][container][iterator] REQUIRE(((void)[](auto x) { ++x; }(i), *i) == *i); } } + + SECTION("bidirectional_iterator") { + // iterator + STATIC_REQUIRE(std::bidirectional_iterator); + { + auto coll = CollectionType(); + coll.create(); + auto a = ++coll.begin(); + REQUIRE(std::addressof(--a) == std::addressof(a)); + a = ++coll.begin(); + auto b = ++coll.begin(); + REQUIRE(a == b); + REQUIRE(a-- == b); + a = ++coll.begin(); + REQUIRE(a == b); + a--; + --b; + REQUIRE(a == b); + a = ++coll.begin(); + b = ++coll.begin(); + REQUIRE(a == b); + REQUIRE(--(++a) == b); + REQUIRE(++(--a) == b); + } + // const_iterator + STATIC_REQUIRE(std::bidirectional_iterator); + auto coll = CollectionType(); + coll.create(); + auto a = ++coll.cbegin(); + REQUIRE(std::addressof(--a) == std::addressof(a)); + a = ++coll.cbegin(); + auto b = ++coll.cbegin(); + REQUIRE(a == b); + REQUIRE(a-- == b); + a = ++coll.cbegin(); + REQUIRE(a == b); + a--; + --b; + REQUIRE(a == b); + a = ++coll.cbegin(); + b = ++coll.cbegin(); + REQUIRE(a == b); + REQUIRE(--(++a) == b); + REQUIRE(++(--a) == b); + } } TEST_CASE("Collection and unsupported iterator concepts", "[collection][container][iterator][std]") { @@ -539,9 +584,6 @@ TEST_CASE("Collection and unsupported iterator concepts", "[collection][containe DOCUMENTED_STATIC_FAILURE(std::output_iterator); DOCUMENTED_STATIC_FAILURE(std::output_iterator); DOCUMENTED_STATIC_FAILURE(std::output_iterator); - // std::bidirectional_iterator - DOCUMENTED_STATIC_FAILURE(std::bidirectional_iterator); - DOCUMENTED_STATIC_FAILURE(std::bidirectional_iterator); // std::random_access_iterator DOCUMENTED_STATIC_FAILURE(std::random_access_iterator); DOCUMENTED_STATIC_FAILURE(std::random_access_iterator); @@ -950,15 +992,63 @@ TEST_CASE("Collection and std iterator adaptors", "[collection][container][adapt STATIC_REQUIRE(traits::has_iterator_category_v>); DOCUMENTED_STATIC_FAILURE( std::is_base_of_v::iterator_category>); - DOCUMENTED_STATIC_FAILURE(std::bidirectional_iterator); - // TODO add runtime checks here + STATIC_REQUIRE(std::bidirectional_iterator); + { + auto coll = CollectionType(); + coll.create().cellID(42); + coll.create().cellID(43); + auto rit = std::reverse_iterator(std::end(coll)); + DOCUMENTED_STATIC_FAILURE( + traits::has_member_of_pointer_v); // can't -> because + // std::reverse_iterator::operator->() + // is const + // REQUIRE(rit->cellID() == 43); + REQUIRE((*rit).cellID() == 43); + ++rit; + // REQUIRE(rit->cellID() == 42); + REQUIRE((*rit).cellID() == 42); + REQUIRE(rit.base()->cellID() == 43); + rit = std::reverse_iterator(std::begin(coll)); + REQUIRE(rit.base()->cellID() == 42); + --rit; + // REQUIRE(rit->cellID() == 42); + REQUIRE((*rit).cellID() == 42); + REQUIRE(rit.base()->cellID() == 43); + --rit; + // REQUIRE(rit->cellID() == 43); + REQUIRE((*rit).cellID() == 43); + } // const_iterator STATIC_REQUIRE(traits::has_const_iterator_v); STATIC_REQUIRE(traits::has_iterator_category_v>); DOCUMENTED_STATIC_FAILURE( std::is_base_of_v::iterator_category>); - DOCUMENTED_STATIC_FAILURE(std::bidirectional_iterator); - // TODO add runtime checks here + STATIC_REQUIRE(std::bidirectional_iterator); + { + auto coll = CollectionType(); + coll.create().cellID(42); + coll.create().cellID(43); + auto rit = std::reverse_iterator(std::cend(coll)); + DOCUMENTED_STATIC_FAILURE( + traits::has_member_of_pointer_v); // can't -> because + // std::reverse_iterator::operator->() + // is const + // REQUIRE(rit->cellID() == 43); + REQUIRE((*rit).cellID() == 43); + ++rit; + // REQUIRE(rit->cellID() == 42); + REQUIRE((*rit).cellID() == 42); + REQUIRE(rit.base()->cellID() == 43); + rit = std::reverse_iterator(std::cbegin(coll)); + REQUIRE(rit.base()->cellID() == 42); + --rit; + // REQUIRE(rit->cellID() == 42); + REQUIRE((*rit).cellID() == 42); + REQUIRE(rit.base()->cellID() == 43); + --rit; + // REQUIRE(rit->cellID() == 43); + REQUIRE((*rit).cellID() == 43); + } } SECTION("Back inserter") { DOCUMENTED_STATIC_FAILURE(traits::has_const_reference_v); @@ -1115,7 +1205,7 @@ TEST_CASE("Collection as range", "[collection][ranges][std]") { // std::range::forward_range STATIC_REQUIRE(std::ranges::forward_range); // std::range::bidirectional_range - DOCUMENTED_STATIC_FAILURE(std::ranges::bidirectional_range); + STATIC_REQUIRE(std::ranges::bidirectional_range); // std::range::random_access_range DOCUMENTED_STATIC_FAILURE(std::ranges::random_access_range); // std::range::contiguous_range @@ -1189,6 +1279,9 @@ TEST_CASE("Collection and std ranges algorithms", "[collection][ranges][std]") { auto target = std::begin(coll); std::ranges::advance(target, 2); REQUIRE(adjacent_it == target); + + auto rev_view = std::ranges::views::reverse(coll); + REQUIRE(rev_view.front().cellID() == 3); } // helper concept for unsupported algorithm compilation test From a8835c70ee04e970dbca269797ae29229d14175a Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Sat, 14 Dec 2024 18:39:33 +0100 Subject: [PATCH 03/14] add reverse begin and end methods to collection --- python/templates/Collection.h.jinja2 | 22 ++++++++++++++++++++++ tests/unittests/unittest.cpp | 20 ++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/python/templates/Collection.h.jinja2 b/python/templates/Collection.h.jinja2 index 53be2ddb3..892341e11 100644 --- a/python/templates/Collection.h.jinja2 +++ b/python/templates/Collection.h.jinja2 @@ -50,6 +50,8 @@ public: using iterator = {{ class.bare_type }}MutableCollectionIterator; using difference_type = ptrdiff_t; using size_type = size_t; + using const_reverse_iterator = std::reverse_iterator; + using reverse_iterator = std::reverse_iterator; {{ class.bare_type }}Collection(); {{ class.bare_type }}Collection({{ class.bare_type }}CollectionData&& data, bool isSubsetColl); @@ -167,6 +169,26 @@ public: const_iterator cend() const { return end(); } + // reverse iterators + reverse_iterator rbegin() { + return reverse_iterator(end()); + } + const_reverse_iterator rbegin() const { + return const_reverse_iterator(end()); + } + const_reverse_iterator crbegin() const { + return rbegin(); + } + reverse_iterator rend() { + return reverse_iterator(begin()); + } + const_reverse_iterator rend() const { + return const_reverse_iterator(begin()); + } + const_reverse_iterator crend() const { + return rend(); + } + {% for member in Members %} std::vector<{{ member.full_type }}> {{ member.name }}(const size_t nElem = 0) const; diff --git a/tests/unittests/unittest.cpp b/tests/unittests/unittest.cpp index 0e0210915..ca3f8bf54 100644 --- a/tests/unittests/unittest.cpp +++ b/tests/unittests/unittest.cpp @@ -206,6 +206,26 @@ TEST_CASE("Looping", "[basics]") { } } +TEST_CASE("Reverse iterators", "[basics]") { + auto coll = ExampleHitCollection(); + coll.create(); + coll.create(); + auto it = std::rbegin(coll); + (*it).energy(43); + (*++it).energy(42); + REQUIRE((*it).energy() == 42); + REQUIRE((*--it).energy() == 43); + it = std::rend(coll); + REQUIRE((*--it).energy() == 42); + REQUIRE((*--it).energy() == 43); + auto cit = std::crbegin(coll); + REQUIRE((*cit).energy() == 43); + REQUIRE((*++cit).energy() == 42); + cit = std::crend(coll); + REQUIRE((*--cit).energy() == 42); + REQUIRE((*--cit).energy() == 43); +} + TEST_CASE("Notebook", "[basics]") { auto hits = ExampleHitCollection(); for (unsigned i = 0; i < 12; ++i) { From 1d7a4c066dceea92d68563c11d2a0351cfcc92f1 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Sun, 15 Dec 2024 00:05:02 +0100 Subject: [PATCH 04/14] add support for `std::random_access_iterator` --- doc/collections_as_container.md | 8 +- python/templates/macros/iterator.jinja2 | 47 ++++++- tests/unittests/std_interoperability.cpp | 164 ++++++++++++++++++++++- 3 files changed, 208 insertions(+), 11 deletions(-) diff --git a/doc/collections_as_container.md b/doc/collections_as_container.md index 0bb6a495c..f045d77cb 100644 --- a/doc/collections_as_container.md +++ b/doc/collections_as_container.md @@ -90,7 +90,7 @@ In the following tables a convention from `Collection` is used: `iterator` stand | `std::output_iterator` | ❌ no | ❌ no | | `std::forward_iterator` | ✔️ yes (see note below) | ✔️ yes (see note below) | | `std::bidirectional_iterator` | ✔️ yes | ✔️ yes | -| `std::random_access_iterator` | ❌ no | ❌ no | +| `std::random_access_iterator` | ✔️ yes | ✔️ yes | | `std::contiguous_iterator` | ❌ no | ❌ no | > [!NOTE] @@ -186,7 +186,7 @@ In addition to the *LegacyForwardIterator* the C++ standard specifies also the * | `std::ranges::output_range` | ❌ no | | `std::ranges::forward_range` | ✔️ yes | | `std::ranges::bidirectional_range` | ✔️ yes | -| `std::ranges::random_access_range` | ❌ no | +| `std::ranges::random_access_range` | ✔️ yes | | `std::ranges::contiguous_range` | ❌ no | | `std::ranges::common_range` | ✔️ yes | | `std::ranges::viewable_range` | ✔️ yes | @@ -211,11 +211,11 @@ std::sort(std::begin(collection), std::end(collection)); // requires RandomAcces The arguments of standard range algorithms are checked at compile time and must fulfil certain iterator concepts, such as `std::input_iterator` or `std::ranges::input_range`. -The iterators of PODIO collections model the `std::bidirectional_iterator` concept, so range algorithms that require this iterator type will work correctly with PODIO iterators. If an algorithm compiles, it is guaranteed to work as expected. +The iterators of PODIO collections model the `std::random_access_iterator` concept, so range algorithms that require this iterator type will work correctly with PODIO iterators. If an algorithm compiles, it is guaranteed to work as expected. In particular, the PODIO collections' iterators do not fulfil the `std::output_iterator` concept, and as a result, mutating algorithms relying on this iterator type will not compile. -Similarly the collections themselves model the `std::bidirectional_range` concept and can be used in the range algorithms that require that concept. The algorithms requiring unsupported range concept, such as `std::output_range`, won't compile. +Similarly the collections themselves model the `std::random_access_range` concept and can be used in the range algorithms that require that concept. The algorithms requiring unsupported range concept, such as `std::output_range`, won't compile. For example: ```c++ diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index 803c475fe..f56b7287c 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -10,7 +10,7 @@ public: using iterator_category = std::input_iterator_tag; // `std::forward_iterator` is supported except that the pointers obtained with `operator->()` // remain valid as long as the iterator is valid, not as long as the range is valid. - using iterator_concept = std::bidirectional_iterator_tag; + using iterator_concept = std::random_access_iterator_tag; {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object({{ ptr_init }}), m_collection(collection) {} {{ iterator_type }}() = default; @@ -21,8 +21,8 @@ public: {{ iterator_type }}& operator=({{iterator_type}}&&) = default; ~{{ iterator_type }}() = default; - bool operator!=(const {{ iterator_type}}& x) const { - return m_index != x.m_index; + auto operator<=>(const {{ iterator_type}}& other) const { + return m_index <=> other.m_index; } bool operator==(const {{ iterator_type }}& x) const { @@ -35,6 +35,13 @@ public: {{ iterator_type }} operator++(int); {{ iterator_type }}& operator--(); {{ iterator_type }} operator--(int); + {{ iterator_type }}& operator+=(difference_type n); + {{ iterator_type }} operator+(difference_type n) const; + friend {{ iterator_type }} operator+(difference_type n, const {{ iterator_type }}& it); + {{ iterator_type }}& operator-=(difference_type n); + {{ iterator_type }} operator-(difference_type n) const; + reference operator[](difference_type n) const; + difference_type operator-(const {{ iterator_type }}& other) const; private: size_t m_index{0}; @@ -79,5 +86,39 @@ private: return copy; } +{{ iterator_type }}& {{ iterator_type }}::operator+=(difference_type n) { + m_index += n; + return *this; +} + +{{ iterator_type }} {{ iterator_type }}::operator+(difference_type n) const { + auto copy = *this; + copy += n; + return copy; +} + +{{ iterator_type }} operator+({{ iterator_type }}::difference_type n, const {{ iterator_type }}& it) { + return it + n; +} + +{{ iterator_type }}& {{ iterator_type }}::operator-=(difference_type n) { + m_index -= n; + return *this; +} + +{{ iterator_type }} {{ iterator_type }}::operator-(difference_type n) const { + auto copy = *this; + copy -= n; + return copy; +} + +{{ iterator_type }}::reference {{ iterator_type }}::operator[](difference_type n) const { + return reference{ {{ ptr_type }}((*m_collection)[m_index + n]) }; +} + +{{ iterator_type }}::difference_type {{ iterator_type }}::operator-(const {{ iterator_type }}& other) const { + return m_index - other.m_index; +} + {% endwith %} {% endmacro %} diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index 93960a4d5..ec1cdaeeb 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -571,6 +571,165 @@ TEST_CASE("Collection and iterator concepts", "[collection][container][iterator] REQUIRE(--(++a) == b); REQUIRE(++(--a) == b); } + + SECTION("random_access_iterator") { + // iterator + STATIC_REQUIRE(std::totally_ordered); + { + auto coll = CollectionType(); + coll.create(); + coll.create(); + coll.create(); + auto a = coll.begin(); + auto b = coll.begin(); + REQUIRE(!(a < b)); + REQUIRE(!(a > b)); + REQUIRE((a == b)); + b = ++coll.begin(); + REQUIRE((a < b)); + REQUIRE(!(a > b)); + REQUIRE(!(a == b)); + a = ++coll.begin(); + b = coll.begin(); + REQUIRE(!(a < b)); + REQUIRE(a > b); + REQUIRE(!(a == b)); + auto c = coll.begin(); + a = c++; + b = c++; + REQUIRE(a < b); + REQUIRE(b < c); + REQUIRE(a < c); + REQUIRE((a > b) == (b < a)); + REQUIRE((a >= b) == !(a < b)); + REQUIRE((a <= b) == !(a > b)); + } + STATIC_REQUIRE(std::sized_sentinel_for); + { + auto coll = CollectionType(); + coll.create(); + auto i = coll.begin(); + auto s = coll.end(); + auto n = 1; + REQUIRE(s - i == n); + REQUIRE(i - s == -n); + } + STATIC_REQUIRE(std::random_access_iterator); + { + auto coll = CollectionType(); + coll.create().cellID(42); + coll.create().cellID(43); + coll.create().cellID(44); + coll.create().cellID(45); + auto a = coll.begin(); + auto n = 2; + auto b = a + n; + REQUIRE((a += n) == b); + a = coll.begin(); + REQUIRE(std::addressof(a += n) == std::addressof(a)); + a = coll.begin(); + auto k = a + n; + REQUIRE(k == (a += n)); + a = coll.begin(); + REQUIRE((a + n) == (n + a)); + auto x = 1; + auto y = 2; + REQUIRE((a + (x + y)) == ((a + x) + y)); + REQUIRE((a + 0) == a); + b = a + n; + REQUIRE((--b) == (a + n - 1)); + b = a + n; + REQUIRE((b += -n) == a); + b = a + n; + REQUIRE((b -= +n) == a); + b = a + n; + REQUIRE(std::addressof(b -= n) == std::addressof(b)); + b = a + n; + k = b - n; + REQUIRE(k == (b -= n)); + b = a + n; + REQUIRE(a[n] == *b); + REQUIRE(a <= b); + } + // const_iterator + STATIC_REQUIRE(std::totally_ordered); + { + auto coll = CollectionType(); + coll.create(); + coll.create(); + coll.create(); + auto a = coll.cbegin(); + auto b = coll.cbegin(); + REQUIRE(!(a < b)); + REQUIRE(!(a > b)); + REQUIRE((a == b)); + b = ++coll.cbegin(); + REQUIRE((a < b)); + REQUIRE(!(a > b)); + REQUIRE(!(a == b)); + a = ++coll.cbegin(); + b = coll.cbegin(); + REQUIRE(!(a < b)); + REQUIRE(a > b); + REQUIRE(!(a == b)); + auto c = coll.cbegin(); + a = c++; + b = c++; + REQUIRE(a < b); + REQUIRE(b < c); + REQUIRE(a < c); + REQUIRE((a > b) == (b < a)); + REQUIRE((a >= b) == !(a < b)); + REQUIRE((a <= b) == !(a > b)); + } + STATIC_REQUIRE(std::sized_sentinel_for); + { + auto coll = CollectionType(); + coll.create(); + auto i = coll.cbegin(); + auto s = coll.cend(); + auto n = 1; + REQUIRE(s - i == n); + REQUIRE(i - s == -n); + } + STATIC_REQUIRE(std::random_access_iterator); + { + auto coll = CollectionType(); + coll.create().cellID(42); + coll.create().cellID(43); + coll.create().cellID(44); + coll.create().cellID(45); + auto a = coll.cbegin(); + auto n = 2; + auto b = a + n; + REQUIRE((a += n) == b); + a = coll.cbegin(); + REQUIRE(std::addressof(a += n) == std::addressof(a)); + a = coll.cbegin(); + auto k = a + n; + REQUIRE(k == (a += n)); + a = coll.cbegin(); + REQUIRE((a + n) == (n + a)); + auto x = 1; + auto y = 2; + REQUIRE((a + (x + y)) == ((a + x) + y)); + REQUIRE((a + 0) == a); + b = a + n; + REQUIRE((--b) == (a + n - 1)); + b = a + n; + REQUIRE((b += -n) == a); + b = a + n; + REQUIRE((b -= +n) == a); + b = a + n; + REQUIRE(std::addressof(b -= n) == std::addressof(b)); + b = a + n; + k = b - n; + REQUIRE(k == (b -= n)); + b = a + n; + REQUIRE(a[n] == *b); + REQUIRE(a <= b); + } + } } TEST_CASE("Collection and unsupported iterator concepts", "[collection][container][iterator][std]") { @@ -584,9 +743,6 @@ TEST_CASE("Collection and unsupported iterator concepts", "[collection][containe DOCUMENTED_STATIC_FAILURE(std::output_iterator); DOCUMENTED_STATIC_FAILURE(std::output_iterator); DOCUMENTED_STATIC_FAILURE(std::output_iterator); - // std::random_access_iterator - DOCUMENTED_STATIC_FAILURE(std::random_access_iterator); - DOCUMENTED_STATIC_FAILURE(std::random_access_iterator); // std::contiguous_iterator DOCUMENTED_STATIC_FAILURE(std::contiguous_iterator); DOCUMENTED_STATIC_FAILURE(std::contiguous_iterator); @@ -1207,7 +1363,7 @@ TEST_CASE("Collection as range", "[collection][ranges][std]") { // std::range::bidirectional_range STATIC_REQUIRE(std::ranges::bidirectional_range); // std::range::random_access_range - DOCUMENTED_STATIC_FAILURE(std::ranges::random_access_range); + STATIC_REQUIRE(std::ranges::random_access_range); // std::range::contiguous_range DOCUMENTED_STATIC_FAILURE(std::ranges::contiguous_range); // std::range::common_range From d7184537ae34cdf44b16a5307f71d8cb34c3c12c Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Fri, 20 Dec 2024 00:26:13 +0100 Subject: [PATCH 05/14] add more detailed checks for `contiguous_iterator` --- tests/unittests/std_interoperability.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index ec1cdaeeb..01e71f8b7 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -744,7 +744,11 @@ TEST_CASE("Collection and unsupported iterator concepts", "[collection][containe DOCUMENTED_STATIC_FAILURE(std::output_iterator); DOCUMENTED_STATIC_FAILURE(std::output_iterator); // std::contiguous_iterator + DOCUMENTED_STATIC_FAILURE(std::is_lvalue_reference_v); + DOCUMENTED_STATIC_FAILURE(std::same_as>); DOCUMENTED_STATIC_FAILURE(std::contiguous_iterator); + DOCUMENTED_STATIC_FAILURE(std::is_lvalue_reference_v); + STATIC_REQUIRE(std::same_as>); DOCUMENTED_STATIC_FAILURE(std::contiguous_iterator); } From ae9b7ad1526aaca8e9a3cb32be4f28415a7c6049 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Fri, 20 Dec 2024 10:33:11 +0100 Subject: [PATCH 06/14] fix format --- doc/collections_as_container.md | 4 ++-- python/templates/Collection.h.jinja2 | 2 +- tests/unittests/std_interoperability.cpp | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/doc/collections_as_container.md b/doc/collections_as_container.md index f045d77cb..7bf37c7bd 100644 --- a/doc/collections_as_container.md +++ b/doc/collections_as_container.md @@ -93,8 +93,8 @@ In the following tables a convention from `Collection` is used: `iterator` stand | `std::random_access_iterator` | ✔️ yes | ✔️ yes | | `std::contiguous_iterator` | ❌ no | ❌ no | -> [!NOTE] ->The collections' iterators fulfil the `std::forward_iterator` except that the pointers obtained with `->` remain valid only as long as the iterator is valid instead of as long as the range remain valid. In practice this means a `ptr` obtained with `auto* ptr = it.operator->();` is valid only as long as `it` is valid. +> [!NOTE] +>The collections' iterators fulfil the `std::forward_iterator` except that the pointers obtained with `->` remain valid only as long as the iterator is valid instead of as long as the range remain valid. In practice this means a `ptr` obtained with `auto* ptr = it.operator->();` is valid only as long as `it` is valid. >The values obtained immediately through `->` (for instance `auto& e = it->energy();`) behaves as expected for `std::forward_iterator` as their validity is tied to the validity of a collection. ### LegacyIterator diff --git a/python/templates/Collection.h.jinja2 b/python/templates/Collection.h.jinja2 index 892341e11..1ee2364c7 100644 --- a/python/templates/Collection.h.jinja2 +++ b/python/templates/Collection.h.jinja2 @@ -187,7 +187,7 @@ public: } const_reverse_iterator crend() const { return rend(); - } + } {% for member in Members %} diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index 01e71f8b7..e81904b05 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -745,10 +745,10 @@ TEST_CASE("Collection and unsupported iterator concepts", "[collection][containe DOCUMENTED_STATIC_FAILURE(std::output_iterator); // std::contiguous_iterator DOCUMENTED_STATIC_FAILURE(std::is_lvalue_reference_v); - DOCUMENTED_STATIC_FAILURE(std::same_as>); + DOCUMENTED_STATIC_FAILURE(std::same_as>); DOCUMENTED_STATIC_FAILURE(std::contiguous_iterator); DOCUMENTED_STATIC_FAILURE(std::is_lvalue_reference_v); - STATIC_REQUIRE(std::same_as>); + STATIC_REQUIRE(std::same_as>); DOCUMENTED_STATIC_FAILURE(std::contiguous_iterator); } From 1e7f6482726133048f182aefed3dfc68cd5626d1 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Mon, 20 Jan 2025 14:34:41 +0100 Subject: [PATCH 07/14] make `LinkCollectionIterator` fulfill `std::forward_iterator` --- include/podio/detail/LinkCollectionIterator.h | 4 +++- tests/unittests/std_interoperability.cpp | 5 ++++- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/include/podio/detail/LinkCollectionIterator.h b/include/podio/detail/LinkCollectionIterator.h index b51c2fb40..953ef1837 100644 --- a/include/podio/detail/LinkCollectionIterator.h +++ b/include/podio/detail/LinkCollectionIterator.h @@ -17,7 +17,9 @@ class LinkCollectionIteratorT { using reference = LinkType; using pointer = LinkType*; using iterator_category = std::input_iterator_tag; - using iterator_concept = std::input_iterator_tag; + // `std::forward_iterator` is supported except that the pointers obtained with `operator->()` + // remain valid as long as the iterator is valid, not as long as the range is valid. + using iterator_concept = std::forward_iterator_tag; LinkCollectionIteratorT(size_t index, const LinkObjPointerContainer* coll) : m_index(index), m_object(podio::utils::MaybeSharedPtr{nullptr}), m_collection(coll) { diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index e81904b05..17dcce8fe 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -1459,18 +1459,21 @@ TEST_CASE("Collection and unsupported std ranges algorithms", "[collection][rang DOCUMENTED_STATIC_FAILURE(is_range_fillable); } -TEST_CASE("LinkCollectionIterator and iterator concepts", "[links][iterator][std]") { +TEST_CASE("LinkCollectionIterator and iterator concepts", "[links][ranges][std]") { using link_iterator = podio::LinkCollectionIteratorT; using link_const_iterator = podio::LinkCollectionIteratorT; STATIC_REQUIRE(std::input_iterator); STATIC_REQUIRE(std::input_iterator); + STATIC_REQUIRE(std::forward_iterator); + STATIC_REQUIRE(std::forward_iterator); } TEST_CASE("LinkCollection and range concepts", "[links][iterator][std]") { using link_collection = podio::LinkCollection; STATIC_REQUIRE(std::ranges::input_range); + STATIC_REQUIRE(std::ranges::forward_range); STATIC_REQUIRE(std::ranges::sized_range); STATIC_REQUIRE(std::ranges::common_range); STATIC_REQUIRE(std::ranges::viewable_range); From dc1bc4a4b50a0c1819227e809b86252facce418c Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Mon, 20 Jan 2025 14:40:42 +0100 Subject: [PATCH 08/14] make `LinkCollectionIterator` fulfill `std::bidirectional_iterator` --- include/podio/detail/LinkCollectionIterator.h | 13 +++++- tests/unittests/std_interoperability.cpp | 46 +++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/include/podio/detail/LinkCollectionIterator.h b/include/podio/detail/LinkCollectionIterator.h index 953ef1837..d78a71665 100644 --- a/include/podio/detail/LinkCollectionIterator.h +++ b/include/podio/detail/LinkCollectionIterator.h @@ -19,7 +19,7 @@ class LinkCollectionIteratorT { using iterator_category = std::input_iterator_tag; // `std::forward_iterator` is supported except that the pointers obtained with `operator->()` // remain valid as long as the iterator is valid, not as long as the range is valid. - using iterator_concept = std::forward_iterator_tag; + using iterator_concept = std::bidirectional_iterator_tag; LinkCollectionIteratorT(size_t index, const LinkObjPointerContainer* coll) : m_index(index), m_object(podio::utils::MaybeSharedPtr{nullptr}), m_collection(coll) { @@ -59,6 +59,17 @@ class LinkCollectionIteratorT { return copy; } + LinkCollectionIteratorT& operator--() { + --m_index; + return *this; + } + + LinkCollectionIteratorT operator--(int) { + auto copy = *this; + --m_index; + return copy; + } + private: size_t m_index{0}; LinkType m_object{podio::utils::MaybeSharedPtr{nullptr}}; diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index 17dcce8fe..844aef4ee 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -1462,11 +1462,56 @@ TEST_CASE("Collection and unsupported std ranges algorithms", "[collection][rang TEST_CASE("LinkCollectionIterator and iterator concepts", "[links][ranges][std]") { using link_iterator = podio::LinkCollectionIteratorT; using link_const_iterator = podio::LinkCollectionIteratorT; + using link_collection = podio::LinkCollection; STATIC_REQUIRE(std::input_iterator); STATIC_REQUIRE(std::input_iterator); STATIC_REQUIRE(std::forward_iterator); STATIC_REQUIRE(std::forward_iterator); + SECTION("bidirectional_iterator") { + STATIC_REQUIRE(std::bidirectional_iterator); + { + auto coll = link_collection(); + coll.create(); + auto a = ++coll.begin(); + REQUIRE(std::addressof(--a) == std::addressof(a)); + a = ++coll.begin(); + auto b = ++coll.begin(); + REQUIRE(a == b); + REQUIRE(a-- == b); + a = ++coll.begin(); + REQUIRE(a == b); + a--; + --b; + REQUIRE(a == b); + a = ++coll.begin(); + b = ++coll.begin(); + REQUIRE(a == b); + REQUIRE(--(++a) == b); + REQUIRE(++(--a) == b); + } + STATIC_REQUIRE(std::bidirectional_iterator); + { + auto coll = link_collection(); + coll.create(); + auto a = ++coll.cbegin(); + REQUIRE(std::addressof(--a) == std::addressof(a)); + a = ++coll.cbegin(); + auto b = ++coll.cbegin(); + REQUIRE(a == b); + REQUIRE(a-- == b); + a = ++coll.cbegin(); + REQUIRE(a == b); + a--; + --b; + REQUIRE(a == b); + a = ++coll.cbegin(); + b = ++coll.cbegin(); + REQUIRE(a == b); + REQUIRE(--(++a) == b); + REQUIRE(++(--a) == b); + } + } } TEST_CASE("LinkCollection and range concepts", "[links][iterator][std]") { @@ -1474,6 +1519,7 @@ TEST_CASE("LinkCollection and range concepts", "[links][iterator][std]") { STATIC_REQUIRE(std::ranges::input_range); STATIC_REQUIRE(std::ranges::forward_range); + STATIC_REQUIRE(std::ranges::bidirectional_range); STATIC_REQUIRE(std::ranges::sized_range); STATIC_REQUIRE(std::ranges::common_range); STATIC_REQUIRE(std::ranges::viewable_range); From 16305633b79c31725dcd3c282fa40979ee0fc674 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Mon, 20 Jan 2025 14:55:00 +0100 Subject: [PATCH 09/14] add reverse iterators methods to `LinkCollection` and `UserDataCollection` --- include/podio/UserDataCollection.h | 21 +++++++++++++++++++++ include/podio/detail/LinkCollectionImpl.h | 23 +++++++++++++++++++++++ tests/unittests/links.cpp | 13 +++++++++++++ tests/unittests/unittest.cpp | 19 +++++++++++++++++++ 4 files changed, 76 insertions(+) diff --git a/include/podio/UserDataCollection.h b/include/podio/UserDataCollection.h index d49a07011..dae5c4c40 100644 --- a/include/podio/UserDataCollection.h +++ b/include/podio/UserDataCollection.h @@ -81,6 +81,8 @@ class UserDataCollection : public CollectionBase { using iterator = typename std::vector::iterator; using difference_type = typename std::vector::difference_type; using size_type = typename std::vector::size_type; + using const_reverse_iterator = typename std::vector::const_reverse_iterator; + using reverse_iterator = typename std::vector::reverse_iterator; UserDataCollection() = default; /// Constructor from an existing vector (which will be moved from!) @@ -222,6 +224,25 @@ class UserDataCollection : public CollectionBase { const_iterator cend() const { return _vec.cend(); } + // reverse iterators + reverse_iterator rbegin() { + return _vec.rbegin(); + } + const_reverse_iterator rbegin() const { + return _vec.rbegin(); + } + const_reverse_iterator crbegin() const { + return _vec.crbegin(); + } + reverse_iterator rend() { + return _vec.rend(); + } + const_reverse_iterator rend() const { + return _vec.rend(); + } + const_reverse_iterator crend() const { + return _vec.crend(); + } typename std::vector::reference operator[](size_t idx) { return _vec[idx]; diff --git a/include/podio/detail/LinkCollectionImpl.h b/include/podio/detail/LinkCollectionImpl.h index 559d63ea4..8b9cc2425 100644 --- a/include/podio/detail/LinkCollectionImpl.h +++ b/include/podio/detail/LinkCollectionImpl.h @@ -16,6 +16,8 @@ #include "podio/utilities/MaybeSharedPtr.h" #include "podio/utilities/TypeHelpers.h" +#include + #ifdef PODIO_JSON_OUTPUT #include "nlohmann/json.hpp" #endif @@ -48,6 +50,8 @@ class LinkCollection : public podio::CollectionBase { using iterator = LinkMutableCollectionIterator; using difference_type = ptrdiff_t; using size_type = size_t; + using const_reverse_iterator = std::reverse_iterator; + using reverse_iterator = std::reverse_iterator; LinkCollection() = default; @@ -172,6 +176,25 @@ class LinkCollection : public podio::CollectionBase { iterator end() { return iterator(m_storage.entries.size(), &m_storage.entries); } + // reverse iterators + reverse_iterator rbegin() { + return reverse_iterator(end()); + } + const_reverse_iterator rbegin() const { + return const_reverse_iterator(end()); + } + const_reverse_iterator crbegin() const { + return rbegin(); + } + reverse_iterator rend() { + return reverse_iterator(begin()); + } + const_reverse_iterator rend() const { + return const_reverse_iterator(begin()); + } + const_reverse_iterator crend() const { + return rend(); + } bool isValid() const override { return m_isValid; diff --git a/tests/unittests/links.cpp b/tests/unittests/links.cpp index eba6bd5da..27604b71e 100644 --- a/tests/unittests/links.cpp +++ b/tests/unittests/links.cpp @@ -477,6 +477,19 @@ TEST_CASE("Links with interfaces", "[links][interface-types]") { REQUIRE(rLink.get() == cluster); } +TEST_CASE("Links reverse iterators", "[links][iterator]") { + const auto [linkColl, hitColl, clusterColl] = createLinkCollections(); + REQUIRE(linkColl.size() > 1); + auto it = --linkColl.rend(); + REQUIRE(*it == *linkColl.begin()); + it = linkColl.rbegin(); + REQUIRE(*it == *--linkColl.end()); + auto cit = --linkColl.rend(); + REQUIRE(*cit == *linkColl.cbegin()); + cit = linkColl.crbegin(); + REQUIRE(*cit == *--linkColl.cend()); +} + #ifdef PODIO_JSON_OUTPUT TEST_CASE("Link JSON conversion", "[links][json]") { const auto& [links, hits, clusters] = createLinkCollections(); diff --git a/tests/unittests/unittest.cpp b/tests/unittests/unittest.cpp index ca3f8bf54..851287bf5 100644 --- a/tests/unittests/unittest.cpp +++ b/tests/unittests/unittest.cpp @@ -226,6 +226,25 @@ TEST_CASE("Reverse iterators", "[basics]") { REQUIRE((*--cit).energy() == 43); } +TEST_CASE("UserDataCollection reverse iterators", "[basics]") { + auto coll = podio::UserDataCollection(); + coll.push_back(42); + coll.push_back(43); + auto it = std::rbegin(coll); + REQUIRE(*it == 43); + REQUIRE(*++it == 42); + REQUIRE(*--it == 43); + it = std::rend(coll); + REQUIRE(*--it == 42); + REQUIRE(*--it == 43); + auto cit = std::crbegin(coll); + REQUIRE(*cit == 43); + REQUIRE(*++cit == 42); + cit = std::crend(coll); + REQUIRE(*--cit == 42); + REQUIRE(*--cit == 43); +} + TEST_CASE("Notebook", "[basics]") { auto hits = ExampleHitCollection(); for (unsigned i = 0; i < 12; ++i) { From 8f9e65593f5236ad15e3e109af3142ae6ec49a83 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Mon, 20 Jan 2025 15:31:19 +0100 Subject: [PATCH 10/14] make `LinkCollectionIterator` fulfill `std::random_access_iterator` --- include/podio/detail/LinkCollectionIterator.h | 40 +++++++++- tests/unittests/std_interoperability.cpp | 77 +++++++++++++++++++ 2 files changed, 114 insertions(+), 3 deletions(-) diff --git a/include/podio/detail/LinkCollectionIterator.h b/include/podio/detail/LinkCollectionIterator.h index d78a71665..b8bb89c6e 100644 --- a/include/podio/detail/LinkCollectionIterator.h +++ b/include/podio/detail/LinkCollectionIterator.h @@ -19,7 +19,7 @@ class LinkCollectionIteratorT { using iterator_category = std::input_iterator_tag; // `std::forward_iterator` is supported except that the pointers obtained with `operator->()` // remain valid as long as the iterator is valid, not as long as the range is valid. - using iterator_concept = std::bidirectional_iterator_tag; + using iterator_concept = std::random_access_iterator_tag; LinkCollectionIteratorT(size_t index, const LinkObjPointerContainer* coll) : m_index(index), m_object(podio::utils::MaybeSharedPtr{nullptr}), m_collection(coll) { @@ -31,8 +31,8 @@ class LinkCollectionIteratorT { LinkCollectionIteratorT& operator=(LinkCollectionIteratorT&&) = default; ~LinkCollectionIteratorT() = default; - bool operator!=(const LinkCollectionIteratorT& other) const { - return m_index != other.m_index; + auto operator<=>(const LinkCollectionIteratorT& other) const { + return m_index <=> other.m_index; } bool operator==(const LinkCollectionIteratorT& other) const { @@ -70,6 +70,40 @@ class LinkCollectionIteratorT { return copy; } + LinkCollectionIteratorT& operator+=(difference_type n) { + m_index += n; + return *this; + } + + LinkCollectionIteratorT operator+(difference_type n) const { + auto copy = *this; + copy += n; + return copy; + } + + friend LinkCollectionIteratorT operator+(difference_type n, const LinkCollectionIteratorT& it) { + return it + n; + } + + LinkCollectionIteratorT& operator-=(difference_type n) { + m_index -= n; + return *this; + } + + LinkCollectionIteratorT operator-(difference_type n) const { + auto copy = *this; + copy -= n; + return copy; + } + + LinkType operator[](difference_type n) const { + return LinkType{podio::utils::MaybeSharedPtr((*m_collection)[m_index + n])}; + } + + difference_type operator-(const LinkCollectionIteratorT& other) const { + return m_index - other.m_index; + } + private: size_t m_index{0}; LinkType m_object{podio::utils::MaybeSharedPtr{nullptr}}; diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index 844aef4ee..82cc5b8e9 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -1512,6 +1512,82 @@ TEST_CASE("LinkCollectionIterator and iterator concepts", "[links][ranges][std]" REQUIRE(++(--a) == b); } } + SECTION("random_access_iterator") { + STATIC_REQUIRE(std::random_access_iterator); + { + auto coll = link_collection(); + coll.create(); + coll.create(); + coll.create(); + coll.create(); + auto a = coll.begin(); + auto n = 2; + auto b = a + n; + REQUIRE((a += n) == b); + a = coll.begin(); + REQUIRE(std::addressof(a += n) == std::addressof(a)); + a = coll.begin(); + auto k = a + n; + REQUIRE(k == (a += n)); + a = coll.begin(); + REQUIRE((a + n) == (n + a)); + auto x = 1; + auto y = 2; + REQUIRE((a + (x + y)) == ((a + x) + y)); + REQUIRE((a + 0) == a); + b = a + n; + REQUIRE((--b) == (a + n - 1)); + b = a + n; + REQUIRE((b += -n) == a); + b = a + n; + REQUIRE((b -= +n) == a); + b = a + n; + REQUIRE(std::addressof(b -= n) == std::addressof(b)); + b = a + n; + k = b - n; + REQUIRE(k == (b -= n)); + b = a + n; + REQUIRE(a[n] == *b); + REQUIRE(a <= b); + } + STATIC_REQUIRE(std::random_access_iterator); + { + auto coll = link_collection(); + coll.create(); + coll.create(); + coll.create(); + coll.create(); + auto a = coll.cbegin(); + auto n = 2; + auto b = a + n; + REQUIRE((a += n) == b); + a = coll.cbegin(); + REQUIRE(std::addressof(a += n) == std::addressof(a)); + a = coll.cbegin(); + auto k = a + n; + REQUIRE(k == (a += n)); + a = coll.cbegin(); + REQUIRE((a + n) == (n + a)); + auto x = 1; + auto y = 2; + REQUIRE((a + (x + y)) == ((a + x) + y)); + REQUIRE((a + 0) == a); + b = a + n; + REQUIRE((--b) == (a + n - 1)); + b = a + n; + REQUIRE((b += -n) == a); + b = a + n; + REQUIRE((b -= +n) == a); + b = a + n; + REQUIRE(std::addressof(b -= n) == std::addressof(b)); + b = a + n; + k = b - n; + REQUIRE(k == (b -= n)); + b = a + n; + REQUIRE(a[n] == *b); + REQUIRE(a <= b); + } + } } TEST_CASE("LinkCollection and range concepts", "[links][iterator][std]") { @@ -1520,6 +1596,7 @@ TEST_CASE("LinkCollection and range concepts", "[links][iterator][std]") { STATIC_REQUIRE(std::ranges::input_range); STATIC_REQUIRE(std::ranges::forward_range); STATIC_REQUIRE(std::ranges::bidirectional_range); + STATIC_REQUIRE(std::ranges::random_access_range); STATIC_REQUIRE(std::ranges::sized_range); STATIC_REQUIRE(std::ranges::common_range); STATIC_REQUIRE(std::ranges::viewable_range); From 591531320b9d64c15937acfc10bb3add30db7a81 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Mon, 20 Jan 2025 15:41:20 +0100 Subject: [PATCH 11/14] fix tags in LinkCollection tests for iterators and ranges --- tests/unittests/std_interoperability.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index 82cc5b8e9..a30567e6d 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -1459,7 +1459,7 @@ TEST_CASE("Collection and unsupported std ranges algorithms", "[collection][rang DOCUMENTED_STATIC_FAILURE(is_range_fillable); } -TEST_CASE("LinkCollectionIterator and iterator concepts", "[links][ranges][std]") { +TEST_CASE("LinkCollectionIterator and iterator concepts", "[links][iterators][std]") { using link_iterator = podio::LinkCollectionIteratorT; using link_const_iterator = podio::LinkCollectionIteratorT; using link_collection = podio::LinkCollection; @@ -1590,7 +1590,7 @@ TEST_CASE("LinkCollectionIterator and iterator concepts", "[links][ranges][std]" } } -TEST_CASE("LinkCollection and range concepts", "[links][iterator][std]") { +TEST_CASE("LinkCollection and range concepts", "[links][ranges][std]") { using link_collection = podio::LinkCollection; STATIC_REQUIRE(std::ranges::input_range); From ba867a0c436ecfbe7497bb4efacec4a0cb0a9430 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Fri, 7 Feb 2025 09:36:28 +0100 Subject: [PATCH 12/14] fix note forward iterator note --- doc/collections_as_container.md | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/doc/collections_as_container.md b/doc/collections_as_container.md index 7bf37c7bd..5f7e8a9b1 100644 --- a/doc/collections_as_container.md +++ b/doc/collections_as_container.md @@ -89,13 +89,14 @@ In the following tables a convention from `Collection` is used: `iterator` stand | `std::input_iterator` | ✔️ yes | ✔️ yes | | `std::output_iterator` | ❌ no | ❌ no | | `std::forward_iterator` | ✔️ yes (see note below) | ✔️ yes (see note below) | -| `std::bidirectional_iterator` | ✔️ yes | ✔️ yes | -| `std::random_access_iterator` | ✔️ yes | ✔️ yes | +| `std::bidirectional_iterator` | ✔️ yes (see note below) | ✔️ yes (see note below) | +| `std::random_access_iterator` | ✔️ yes (see note below) | ✔️ yes (see note below) | | `std::contiguous_iterator` | ❌ no | ❌ no | -> [!NOTE] ->The collections' iterators fulfil the `std::forward_iterator` except that the pointers obtained with `->` remain valid only as long as the iterator is valid instead of as long as the range remain valid. In practice this means a `ptr` obtained with `auto* ptr = it.operator->();` is valid only as long as `it` is valid. ->The values obtained immediately through `->` (for instance `auto& e = it->energy();`) behaves as expected for `std::forward_iterator` as their validity is tied to the validity of a collection. +:::{note} +The collections' iterators fulfil the `std::forward_iterator` except that the pointers obtained with `->` remain valid only as long as the iterator is valid instead of as long as the range remain valid. In practice this means a `ptr` obtained with `auto* ptr = it.operator->();` is valid only as long as `it` is valid. +The values obtained immediately through `->` (for instance `auto& e = it->energy();`) behaves as expected for `std::forward_iterator` as their validity is tied to the validity of a collection. +::: ### LegacyIterator @@ -207,7 +208,7 @@ std::fill(std::begin(collection), std::end(collection), value ); // requires For std::sort(std::begin(collection), std::end(collection)); // requires RandomAccessIterator and Swappable -> might compile, wrong result ``` -## Standard range algorithms +## Collection and standard range algorithms The arguments of standard range algorithms are checked at compile time and must fulfil certain iterator concepts, such as `std::input_iterator` or `std::ranges::input_range`. From 229f8712a6f299d7b07dd3b60189dd988575ccf3 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Fri, 7 Feb 2025 09:41:03 +0100 Subject: [PATCH 13/14] fix forward_iterator multpass-guarantee test --- tests/unittests/std_interoperability.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/unittests/std_interoperability.cpp b/tests/unittests/std_interoperability.cpp index a30567e6d..b63a14268 100644 --- a/tests/unittests/std_interoperability.cpp +++ b/tests/unittests/std_interoperability.cpp @@ -507,10 +507,12 @@ TEST_CASE("Collection and iterator concepts", "[collection][container][iterator] coll.create(); auto i = coll.begin(); auto j = coll.begin(); + // multi-pass guarantee REQUIRE(i == j); REQUIRE(++i == ++j); i = coll.begin(); - REQUIRE(((void)[](auto x) { ++x; }(i), *i) == *i); + REQUIRE(*i == ((void)[](auto x) { ++x; }(i), *i)); // iterating copy doesn't cause side effects that affect the + // original iterator } // const_iterator STATIC_REQUIRE(std::forward_iterator); @@ -520,10 +522,12 @@ TEST_CASE("Collection and iterator concepts", "[collection][container][iterator] coll.create(); auto i = coll.cbegin(); auto j = coll.cbegin(); + // multi-pass guarantee REQUIRE(i == j); REQUIRE(++i == ++j); i = coll.cbegin(); - REQUIRE(((void)[](auto x) { ++x; }(i), *i) == *i); + REQUIRE(*i == ((void)[](auto x) { ++x; }(i), *i)); // iterating copy doesn't cause side effects that affect the + // original iterator } } From eb6daa94cf991f79af91c3e0a6b71792ad606056 Mon Sep 17 00:00:00 2001 From: Mateusz Jakub Fila Date: Fri, 7 Feb 2025 13:08:12 +0100 Subject: [PATCH 14/14] expand forward iterator fulfilment note --- doc/collections_as_container.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/doc/collections_as_container.md b/doc/collections_as_container.md index 5f7e8a9b1..62bc72d9c 100644 --- a/doc/collections_as_container.md +++ b/doc/collections_as_container.md @@ -94,8 +94,9 @@ In the following tables a convention from `Collection` is used: `iterator` stand | `std::contiguous_iterator` | ❌ no | ❌ no | :::{note} -The collections' iterators fulfil the `std::forward_iterator` except that the pointers obtained with `->` remain valid only as long as the iterator is valid instead of as long as the range remain valid. In practice this means a `ptr` obtained with `auto* ptr = it.operator->();` is valid only as long as `it` is valid. -The values obtained immediately through `->` (for instance `auto& e = it->energy();`) behaves as expected for `std::forward_iterator` as their validity is tied to the validity of a collection. +The collections' iterators fulfil the `std::forward_iterator` except that the pointers obtained with `->` remain valid only as long as the iterator itself is valid instead of as long as the range remains valid. In practice, this means a `ptr` obtained with `auto* ptr = it.operator->();` is valid only as long as `it` is valid. +The values obtained immediately through `->` (for instance, `auto& e = it->energy();`) behave as expected for `std::forward_iterator`, as their validity is tied to the collection's validity. +Despite the exception, the collection iterators are made to still report themselves as fulfilling the forward iterator requirements since the problematic usage (`auto* ptr = it.operator->();`) is rare in normal scenarios. ::: ### LegacyIterator