From 383efc683d12c7172f553020e0ec7c59ac31b42d Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 28 Mar 2022 17:15:26 -0500 Subject: [PATCH 01/23] input_iterator type and unit tests --- python/templates/macros/iterator.jinja2 | 19 +++++++- tests/CMakeLists.txt | 4 +- tests/read_forward_iterators.cpp | 42 +++++++++++++++++ tests/read_input_iterators.cpp | 62 +++++++++++++++++++++++++ 4 files changed, 124 insertions(+), 3 deletions(-) create mode 100644 tests/read_forward_iterators.cpp create mode 100644 tests/read_input_iterators.cpp diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index eaba99ba8..25c833e7d 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -2,18 +2,32 @@ {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} class {{ iterator_type }} { public: + using difference_type = std::ptrdiff_t; + using value_type = {{ class.bare_type }}; + using pointer = {{ class.bare_type }}*; + using reference = {{ class.bare_type }}&; + using iterator_category = std::input_iterator_tag; + {{ 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 }}&) = delete; - {{ iterator_type }}& operator=(const {{ iterator_type }}&) = delete; + {{ 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->(); {{ iterator_type }}& operator++(); + {{ iterator_type }} operator++(int) { + {{ iterator_type }} temp = *this; + ++*this; + return temp; + }; private: size_t m_index; @@ -23,6 +37,7 @@ private: {% endwith %} {% endmacro %} +static_assert(std::forward_iterator<{{ iterator_type }}>); {% macro iterator_definitions(class, prefix='') %} {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9ab80d938..ada3a1bdb 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -28,7 +28,7 @@ function(CREATE_PODIO_TEST sourcefile additional_libs) ) endfunction() -set(root_dependent_tests write.cpp read.cpp read-multiple.cpp relation_range.cpp read_and_write.cpp read_and_write_associated.cpp write_timed.cpp read_timed.cpp) +set(root_dependent_tests write.cpp read.cpp read-multiple.cpp relation_range.cpp read_input_iterators.cpp read_forward_iterators.cpp read_and_write.cpp read_and_write_associated.cpp write_timed.cpp read_timed.cpp) set(root_libs TestDataModelDict podio::podioRootIO) foreach( sourcefile ${root_dependent_tests} ) CREATE_PODIO_TEST(${sourcefile} "${root_libs}") @@ -121,6 +121,8 @@ endif() set_property(TEST read PROPERTY DEPENDS write) set_property(TEST read-multiple PROPERTY DEPENDS write) set_property(TEST read_and_write PROPERTY DEPENDS write) +set_property(TEST read_input_iterators PROPERTY DEPENDS write) +set_property(TEST read_forward_iterators PROPERTY DEPENDS write) set_property(TEST read_timed PROPERTY DEPENDS write_timed) add_executable(check_benchmark_outputs check_benchmark_outputs.cpp) diff --git a/tests/read_forward_iterators.cpp b/tests/read_forward_iterators.cpp new file mode 100644 index 000000000..43e0c75cb --- /dev/null +++ b/tests/read_forward_iterators.cpp @@ -0,0 +1,42 @@ +#include "read_test.h" +// podio specific includes +#include "podio/EventStore.h" +#include "podio/ROOTReader.h" +#include "podio/ROOTWriter.h" + +#ifdef __has_include +# if __has_include() +# include +# endif +# if __has_include() +# include +# endif +#endif + +int main(){ + auto reader = podio::ROOTReader(); + auto store = podio::EventStore(); + reader.openFile("example.root"); + store.setReader(&reader); + + unsigned nEvents = reader.getEntries(); + for(unsigned i=0; i("mcparticles"); + + #if __cpp_lib_ranges + auto is_electron = [](const auto& p){ return p.getPDG() == 11; }; + for (const auto& e: mcparticles | std::views::filter(is_electron)) { + // stuff + } + #endif + + store.clear(); + reader.endOfEvent(); + } + reader.closeFile(); + return 0; +} diff --git a/tests/read_input_iterators.cpp b/tests/read_input_iterators.cpp new file mode 100644 index 000000000..55e2e3be4 --- /dev/null +++ b/tests/read_input_iterators.cpp @@ -0,0 +1,62 @@ +#include "read_test.h" +// podio specific includes +#include "podio/EventStore.h" +#include "podio/ROOTReader.h" +#include "podio/ROOTWriter.h" + +int main(){ + auto reader = podio::ROOTReader(); + auto store = podio::EventStore(); + reader.openFile("example.root"); + store.setReader(&reader); + + unsigned nEvents = reader.getEntries(); + for(unsigned i=0; i("mcparticles"); + + // STL find_if with lambda + const auto mcelectrons = std::find_if( + mcparticles.begin(), mcparticles.end(), + [](const auto& p){ return p.PDG() == 11; }); + + // ranged-based for loop + for (const auto& p: mcparticles) { + if (p.PDG() == 11) { + // do something + } + } + + // loop with post-increment + auto mc_first_electron = mcparticles.end(); + for (auto p = mcparticles.begin(); p != mcparticles.end(); p++) { + if (p->PDG() == 11) { + mc_first_electron = p; + break; + } + } + if (mc_first_electron == mcparticles.end()) { + // no electron found + } + + // loop with pre-increment + mc_first_electron = mcparticles.end(); + for (auto p = mcparticles.begin(); p != mcparticles.end(); ++p) { + if (p->PDG() == 11) { + mc_first_electron = p; + break; + } + } + if (mc_first_electron == mcparticles.end()) { + // no electron found + } + + store.clear(); + reader.endOfEvent(); + } + reader.closeFile(); + return 0; +} From 540eae4aa65c043042cc72d59f0a1699bb3f8fac Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 28 Mar 2022 17:51:14 -0500 Subject: [PATCH 02/23] Make checks happy by using all variables --- tests/read_forward_iterators.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/read_forward_iterators.cpp b/tests/read_forward_iterators.cpp index 43e0c75cb..b1ff1cd1b 100644 --- a/tests/read_forward_iterators.cpp +++ b/tests/read_forward_iterators.cpp @@ -25,10 +25,11 @@ int main(){ std::cout<<"reading event "<("mcparticles"); - #if __cpp_lib_ranges auto is_electron = [](const auto& p){ return p.getPDG() == 11; }; + + auto& mcparticles = store.get("mcparticles"); + for (const auto& e: mcparticles | std::views::filter(is_electron)) { // stuff } From db113f832d4cb610d1b70d42e546c42f97d3e3c6 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 28 Mar 2022 18:26:25 -0500 Subject: [PATCH 03/23] Haha, get past the sanitzer checks --- tests/CTestCustom.cmake | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake index 79a9296d8..08ee0f737 100644 --- a/tests/CTestCustom.cmake +++ b/tests/CTestCustom.cmake @@ -20,6 +20,8 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ check_benchmark_outputs read-multiple read-legacy-files + read_input_iterators + read_forward_iterators write_sio read_sio From dc6c7822332e0149c6450bff05b5e6e625af8071 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 28 Mar 2022 22:53:01 -0500 Subject: [PATCH 04/23] Add reverse iterators; split tests into iterators and views --- python/templates/Collection.cc.jinja2 | 5 + python/templates/Collection.h.jinja2 | 52 ++++++- python/templates/MutableObject.h.jinja2 | 1 + python/templates/Object.h.jinja2 | 1 + python/templates/macros/iterator.jinja2 | 83 ++++++++++- tests/CMakeLists.txt | 6 +- tests/CTestCustom.cmake | 4 +- tests/read_input_iterators.cpp | 62 -------- tests/read_iterators.cpp | 134 ++++++++++++++++++ ...d_forward_iterators.cpp => read_views.cpp} | 5 +- 10 files changed, 277 insertions(+), 76 deletions(-) delete mode 100644 tests/read_input_iterators.cpp create mode 100644 tests/read_iterators.cpp rename tests/{read_forward_iterators.cpp => read_views.cpp} (95%) diff --git a/python/templates/Collection.cc.jinja2 b/python/templates/Collection.cc.jinja2 index b76af221b..fff03b2d6 100644 --- a/python/templates/Collection.cc.jinja2 +++ b/python/templates/Collection.cc.jinja2 @@ -1,6 +1,7 @@ {% import "macros/utils.jinja2" as utils %} {% import "macros/collections.jinja2" as macros %} {% from "macros/iterator.jinja2" import iterator_definitions %} +{% from "macros/iterator.jinja2" import reverse_iterator_definitions %} // AUTOMATICALLY GENERATED FILE - DO NOT EDIT #include "{{ incfolder }}{{ class.bare_type }}Collection.h" @@ -152,6 +153,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..e67c55be7 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,22 @@ 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() { + if constexpr (std::ranges::sized_range) { + reserve(std::ranges::size(range)); + } + 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 +140,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 25c833e7d..04f0bb453 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -6,9 +6,9 @@ public: using value_type = {{ class.bare_type }}; using pointer = {{ class.bare_type }}*; using reference = {{ class.bare_type }}&; - using iterator_category = std::input_iterator_tag; + using iterator_category = std::forward_iterator_tag; - {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {} + {{ 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; @@ -28,6 +28,12 @@ public: ++*this; return temp; }; + {{ iterator_type }}& operator--(); + {{ iterator_type }} operator--(int) { + {{ iterator_type }} temp = *this; + --*this; + return temp; + }; private: size_t m_index; @@ -37,7 +43,50 @@ private: {% endwith %} {% endmacro %} -static_assert(std::forward_iterator<{{ iterator_type }}>); +{% 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 = {{ class.bare_type }}; + using pointer = {{ class.bare_type }}*; + using reference = {{ class.bare_type }}&; + using iterator_category = std::forward_iterator_tag; + + {{ 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->(); + {{ 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' %} @@ -56,5 +105,33 @@ static_assert(std::forward_iterator<{{ iterator_type }}>); 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*() { + m_object.m_obj = (*m_collection)[m_index]; + return m_object; +} + +{{ 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/CMakeLists.txt b/tests/CMakeLists.txt index ada3a1bdb..8c5059a1a 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -28,7 +28,7 @@ function(CREATE_PODIO_TEST sourcefile additional_libs) ) endfunction() -set(root_dependent_tests write.cpp read.cpp read-multiple.cpp relation_range.cpp read_input_iterators.cpp read_forward_iterators.cpp read_and_write.cpp read_and_write_associated.cpp write_timed.cpp read_timed.cpp) +set(root_dependent_tests write.cpp read.cpp read-multiple.cpp relation_range.cpp read_iterators.cpp read_views.cpp read_and_write.cpp read_and_write_associated.cpp write_timed.cpp read_timed.cpp) set(root_libs TestDataModelDict podio::podioRootIO) foreach( sourcefile ${root_dependent_tests} ) CREATE_PODIO_TEST(${sourcefile} "${root_libs}") @@ -121,8 +121,8 @@ endif() set_property(TEST read PROPERTY DEPENDS write) set_property(TEST read-multiple PROPERTY DEPENDS write) set_property(TEST read_and_write PROPERTY DEPENDS write) -set_property(TEST read_input_iterators PROPERTY DEPENDS write) -set_property(TEST read_forward_iterators PROPERTY DEPENDS write) +set_property(TEST read_iterators PROPERTY DEPENDS write) +set_property(TEST read_views PROPERTY DEPENDS write) set_property(TEST read_timed PROPERTY DEPENDS write_timed) add_executable(check_benchmark_outputs check_benchmark_outputs.cpp) diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake index 08ee0f737..f5ec23571 100644 --- a/tests/CTestCustom.cmake +++ b/tests/CTestCustom.cmake @@ -20,8 +20,8 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ check_benchmark_outputs read-multiple read-legacy-files - read_input_iterators - read_forward_iterators + read_iterators + read_views write_sio read_sio diff --git a/tests/read_input_iterators.cpp b/tests/read_input_iterators.cpp deleted file mode 100644 index 55e2e3be4..000000000 --- a/tests/read_input_iterators.cpp +++ /dev/null @@ -1,62 +0,0 @@ -#include "read_test.h" -// podio specific includes -#include "podio/EventStore.h" -#include "podio/ROOTReader.h" -#include "podio/ROOTWriter.h" - -int main(){ - auto reader = podio::ROOTReader(); - auto store = podio::EventStore(); - reader.openFile("example.root"); - store.setReader(&reader); - - unsigned nEvents = reader.getEntries(); - for(unsigned i=0; i("mcparticles"); - - // STL find_if with lambda - const auto mcelectrons = std::find_if( - mcparticles.begin(), mcparticles.end(), - [](const auto& p){ return p.PDG() == 11; }); - - // ranged-based for loop - for (const auto& p: mcparticles) { - if (p.PDG() == 11) { - // do something - } - } - - // loop with post-increment - auto mc_first_electron = mcparticles.end(); - for (auto p = mcparticles.begin(); p != mcparticles.end(); p++) { - if (p->PDG() == 11) { - mc_first_electron = p; - break; - } - } - if (mc_first_electron == mcparticles.end()) { - // no electron found - } - - // loop with pre-increment - mc_first_electron = mcparticles.end(); - for (auto p = mcparticles.begin(); p != mcparticles.end(); ++p) { - if (p->PDG() == 11) { - mc_first_electron = p; - break; - } - } - if (mc_first_electron == mcparticles.end()) { - // no electron found - } - - store.clear(); - reader.endOfEvent(); - } - reader.closeFile(); - return 0; -} diff --git a/tests/read_iterators.cpp b/tests/read_iterators.cpp new file mode 100644 index 000000000..f1bc10545 --- /dev/null +++ b/tests/read_iterators.cpp @@ -0,0 +1,134 @@ +#include "read_test.h" +// podio specific includes +#include "podio/EventStore.h" +#include "podio/ROOTReader.h" +#include "podio/ROOTWriter.h" + +int main(){ + auto reader = podio::ROOTReader(); + auto store = podio::EventStore(); + reader.openFile("example.root"); + store.setReader(&reader); + + unsigned nEvents = reader.getEntries(); + for(unsigned i=0; i("mcparticles"); + + // STL find_if with lambda + const auto mcelectrons = std::find_if( + mcparticles.begin(), mcparticles.end(), + [](const auto& p){ return p.PDG() == 11; }); + + // ranged-based for loop + for (const auto& p: mcparticles) { + if (p.PDG() == 11) { + // do something + } + } + + // forward loop with post-increment + auto mc_first_electron_forward_post = mcparticles.end(); + for (auto p = mcparticles.begin(); p != mcparticles.end(); p++) { + if (p->PDG() == 11) { + mc_first_electron_forward_post = p; + break; + } + } + if (mc_first_electron_forward_post == mcparticles.end()) { + // no electron found + } + + // forward loop with pre-increment + auto mc_first_electron_forward_pre = mcparticles.end(); + for (auto p = mcparticles.begin(); p != mcparticles.end(); ++p) { + if (p->PDG() == 11) { + mc_first_electron_forward_pre = p; + break; + } + } + if (mc_first_electron_forward_pre == mcparticles.end()) { + // no electron found + } + + // forward loop with post-increment + auto mc_first_electron_forward_const_post = mcparticles.cend(); + for (auto p = mcparticles.cbegin(); p != mcparticles.cend(); p++) { + if (p->PDG() == 11) { + mc_first_electron_forward_const_post = p; + break; + } + } + if (mc_first_electron_forward_const_post == mcparticles.cend()) { + // no electron found + } + + // forward loop with pre-increment + auto mc_first_electron_forward_const_pre = mcparticles.cend(); + for (auto p = mcparticles.cbegin(); p != mcparticles.cend(); ++p) { + if (p->PDG() == 11) { + mc_first_electron_forward_const_pre = p; + break; + } + } + if (mc_first_electron_forward_const_pre == mcparticles.cend()) { + // no electron found + } + + // reverse loop with post-increment + auto mc_first_electron_reverse_post = mcparticles.rend(); + for (auto p = mcparticles.rbegin(); p != mcparticles.rend(); p++) { + if (p->PDG() == 11) { + mc_first_electron_reverse_post = p; + break; + } + } + if (mc_first_electron_reverse_post == mcparticles.rend()) { + // no electron found + } + + // reverse loop with pre-increment + auto mc_first_electron_reverse_pre = mcparticles.rend(); + for (auto p = mcparticles.rbegin(); p != mcparticles.rend(); ++p) { + if (p->PDG() == 11) { + mc_first_electron_reverse_pre = p; + break; + } + } + if (mc_first_electron_reverse_pre == mcparticles.rend()) { + // no electron found + } + + // reverse loop with post-increment + auto mc_first_electron_reverse_const_post = mcparticles.crend(); + for (auto p = mcparticles.crbegin(); p != mcparticles.crend(); p++) { + if (p->PDG() == 11) { + mc_first_electron_reverse_const_post = p; + break; + } + } + if (mc_first_electron_reverse_const_post == mcparticles.crend()) { + // no electron found + } + + // reverse loop with pre-increment + auto mc_first_electron_reverse_const_pre = mcparticles.crend(); + for (auto p = mcparticles.crbegin(); p != mcparticles.crend(); ++p) { + if (p->PDG() == 11) { + mc_first_electron_reverse_const_pre = p; + break; + } + } + if (mc_first_electron_reverse_const_pre == mcparticles.crend()) { + // no electron found + } + + store.clear(); + reader.endOfEvent(); + } + reader.closeFile(); + return 0; +} diff --git a/tests/read_forward_iterators.cpp b/tests/read_views.cpp similarity index 95% rename from tests/read_forward_iterators.cpp rename to tests/read_views.cpp index b1ff1cd1b..0aaa6cc70 100644 --- a/tests/read_forward_iterators.cpp +++ b/tests/read_views.cpp @@ -26,10 +26,11 @@ int main(){ processEvent(store, i, reader.currentFileVersion()); #if __cpp_lib_ranges - auto is_electron = [](const auto& p){ return p.getPDG() == 11; }; - auto& mcparticles = store.get("mcparticles"); + auto v = mcparticles | std::views::reverse; + + auto is_electron = [](const auto& p){ return p.getPDG() == 11; }; for (const auto& e: mcparticles | std::views::filter(is_electron)) { // stuff } From f74e64e2eacf1a5f9783dd969e4ab4d7fea4dc92 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 29 Mar 2022 20:22:30 -0500 Subject: [PATCH 05/23] Add prefix to iterator types; change to bidirectional_iterator_tag --- python/templates/macros/iterator.jinja2 | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index 04f0bb453..d01be18b7 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -3,10 +3,10 @@ class {{ iterator_type }} { public: using difference_type = std::ptrdiff_t; - using value_type = {{ class.bare_type }}; - using pointer = {{ class.bare_type }}*; - using reference = {{ class.bare_type }}&; - using iterator_category = std::forward_iterator_tag; + using value_type = {{ prefix }}{{ class.bare_type }}; + using pointer = {{ prefix }}{{ class.bare_type }}*; + using reference = {{ prefix }}{{ class.bare_type }}&; + using iterator_category = std::bidirectional_iterator_tag; {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {}; @@ -48,10 +48,10 @@ private: class {{ iterator_type }} { public: using difference_type = std::ptrdiff_t; - using value_type = {{ class.bare_type }}; - using pointer = {{ class.bare_type }}*; - using reference = {{ class.bare_type }}&; - using iterator_category = std::forward_iterator_tag; + using value_type = {{prefix}}{{ class.bare_type }}; + using pointer = {{prefix}}{{ class.bare_type }}*; + using reference = {{prefix}}{{ class.bare_type }}&; + using iterator_category = std::bidirectional_iterator_tag; {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {}; From d2c783bbdb591e1cab2b722d7a7cbe63b74c8ff3 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 29 Mar 2022 20:23:17 -0500 Subject: [PATCH 06/23] Add iterator default constructor (index 0, nullptr) --- python/templates/macros/iterator.jinja2 | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index d01be18b7..521043584 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -8,7 +8,10 @@ public: using reference = {{ prefix }}{{ class.bare_type }}&; using iterator_category = std::bidirectional_iterator_tag; - {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {}; + {{ 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; @@ -53,7 +56,10 @@ public: using reference = {{prefix}}{{ class.bare_type }}&; using iterator_category = std::bidirectional_iterator_tag; - {{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {}; + {{ 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; From 07fc73ae278fdbbe1f34ebd53963db138edd38e7 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 29 Mar 2022 20:23:36 -0500 Subject: [PATCH 07/23] No getter for PDG in read_views test --- tests/read_views.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/read_views.cpp b/tests/read_views.cpp index 0aaa6cc70..8506b0f05 100644 --- a/tests/read_views.cpp +++ b/tests/read_views.cpp @@ -30,7 +30,7 @@ int main(){ auto v = mcparticles | std::views::reverse; - auto is_electron = [](const auto& p){ return p.getPDG() == 11; }; + auto is_electron = [](const auto& p){ return p.PDG() == 11; }; for (const auto& e: mcparticles | std::views::filter(is_electron)) { // stuff } From 3ae40876717f3a8180fd4718822e627c0611380f Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 29 Mar 2022 20:23:59 -0500 Subject: [PATCH 08/23] Const dereference with mutable m_object --- python/templates/macros/iterator.jinja2 | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index 521043584..dccf76298 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -23,7 +23,7 @@ public: 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) { @@ -40,7 +40,7 @@ public: private: size_t m_index; - {{ prefix }}{{ class.bare_type }} m_object; + mutable {{ prefix }}{{ class.bare_type }} m_object; const {{ class.bare_type }}ObjPointerContainer* m_collection; }; {% endwith %} @@ -71,7 +71,7 @@ public: 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) { @@ -88,7 +88,7 @@ public: private: size_t m_index; - {{ prefix }}{{ class.bare_type }} m_object; + mutable {{ prefix }}{{ class.bare_type }} m_object; const {{ class.bare_type }}ObjPointerContainer* m_collection; }; {% endwith %} @@ -96,7 +96,7 @@ private: {% macro iterator_definitions(class, prefix='') %} {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} -{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() { +{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const { m_object.m_obj = (*m_collection)[m_index]; return m_object; } @@ -120,7 +120,7 @@ private: {% macro reverse_iterator_definitions(class, prefix='') %} {% with iterator_type = class.bare_type + prefix + 'CollectionReverseIterator' %} -{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() { +{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const { m_object.m_obj = (*m_collection)[m_index]; return m_object; } From f83d7bc4b1f5a1abcb4aaae4c64b9d184a0ae01b Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 29 Mar 2022 20:24:23 -0500 Subject: [PATCH 09/23] Full(er) set of unit tests (incl c++23 zip) --- CMakeLists.txt | 2 +- tests/unittest.cpp | 127 +++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 1 deletion(-) 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/tests/unittest.cpp b/tests/unittest.cpp index 635ea91f3..0839de147 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,124 @@ TEST_CASE("const correct iterators on collections", "[const-correctness]") { // objects } +TEST_CASE("Iterator concepts", "[iterator-concepts]") { + // a collection to play with + auto collection = ExampleHitCollection(); + + #ifdef __cpp_lib_ranges + // bidirectional iterator concepts (C++20) + STATIC_REQUIRE(std::bidirectional_iterator); + STATIC_REQUIRE(std::bidirectional_iterator); + STATIC_REQUIRE(std::bidirectional_iterator); + STATIC_REQUIRE(std::bidirectional_iterator); + STATIC_REQUIRE(std::ranges::bidirectional_range); + #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); + + // fill from const + collection.clear(); + collection.create(); + auto const_hit = ExampleHit(0x42ULL, 0., 0., 0., 0.); + //FIXME this should work but we may need a conversion constructor! + //std::fill(collection.begin(), collection.end(), const_hit); + //REQUIRE(collection.begin()->cellID() == 0x42ULL); + // fill from mutable + collection.clear(); + collection.create(); + auto mutable_hit = MutableExampleHit(0x42ULL, 0., 0., 0., 0.); + std::fill(collection.begin(), collection.end(), mutable_hit); + //FIXME + //REQUIRE(collection.begin()->cellID() == 0x42ULL); + + // find in collection + collection.clear(); + collection.create(); + collection.create(0x42ULL, 0., 0., 0., 0.); + auto found = std::find_if(collection.begin(), collection.begin(), + [](const auto& h){ return h.cellID() == 0x42ULL; }); + STATIC_REQUIRE(std::is_same_v); + REQUIRE(found != collection.end()); + //FIXME + //REQUIRE(found->cellID() == 0x42ULL); + + #ifdef __cpp_lib_ranges + // 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) | std::views::reverse) { + 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::reverse(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) | std::views::reverse) { + 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(); From 853423b2173b76797c4c19c9197eec91f499e8ad Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 29 Mar 2022 20:34:43 -0500 Subject: [PATCH 10/23] Single line import of iterator_definitions Co-authored-by: Thomas Madlener --- python/templates/Collection.cc.jinja2 | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/python/templates/Collection.cc.jinja2 b/python/templates/Collection.cc.jinja2 index fff03b2d6..d4898eeed 100644 --- a/python/templates/Collection.cc.jinja2 +++ b/python/templates/Collection.cc.jinja2 @@ -1,7 +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 reverse_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" From a71f643061bc6b72200b07ef316de56bc6178a51 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Wed, 30 Mar 2022 08:27:37 -0500 Subject: [PATCH 11/23] Remove reserve for sized_range since not available for podio collection --- python/templates/Collection.h.jinja2 | 3 --- 1 file changed, 3 deletions(-) diff --git a/python/templates/Collection.h.jinja2 b/python/templates/Collection.h.jinja2 index e67c55be7..b89fb8be0 100644 --- a/python/templates/Collection.h.jinja2 +++ b/python/templates/Collection.h.jinja2 @@ -55,9 +55,6 @@ public: {{ 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() { - if constexpr (std::ranges::sized_range) { - reserve(std::ranges::size(range)); - } for (auto&& item : range) { push_back(item); } From 74e46914671e70f986f984ae33dcef1c2bb14c35 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Wed, 30 Mar 2022 08:28:40 -0500 Subject: [PATCH 12/23] Suggested style change Co-authored-by: Thomas Madlener --- python/templates/macros/iterator.jinja2 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index dccf76298..8ece5a10f 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -64,10 +64,10 @@ public: {{ iterator_type }}(const {{ iterator_type }}&) = default; {{ iterator_type }}& operator=(const {{ iterator_type }}&) = default; - bool operator!=(const {{ iterator_type}}& x) const { + 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 { + bool operator==(const {{ iterator_type }}& x) const { return !operator!=(x); } From 4b5cf9f4218d95965f0a7036cfba7b33dc37de1b Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Wed, 30 Mar 2022 15:46:06 -0500 Subject: [PATCH 13/23] static_assert on iterator_traits for bidirectional_iterator_tag --- tests/unittest.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 0839de147..3e131a3c2 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -507,6 +507,12 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { // a collection to play with auto collection = ExampleHitCollection(); + // bidirectional iterator traits + STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); + #ifdef __cpp_lib_ranges // bidirectional iterator concepts (C++20) STATIC_REQUIRE(std::bidirectional_iterator); From d18b1d6508d5a2c88aa2c268a1057b014c841a0f Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Wed, 30 Mar 2022 15:54:39 -0500 Subject: [PATCH 14/23] dereference operator* const to copy without mutable inside iterator --- python/templates/macros/iterator.jinja2 | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index 8ece5a10f..5e3b4bda8 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -40,7 +40,7 @@ public: private: size_t m_index; - mutable {{ prefix }}{{ class.bare_type }} m_object; + {{ prefix }}{{ class.bare_type }} m_object; const {{ class.bare_type }}ObjPointerContainer* m_collection; }; {% endwith %} @@ -88,7 +88,7 @@ public: private: size_t m_index; - mutable {{ prefix }}{{ class.bare_type }} m_object; + {{ prefix }}{{ class.bare_type }} m_object; const {{ class.bare_type }}ObjPointerContainer* m_collection; }; {% endwith %} @@ -97,8 +97,7 @@ private: {% macro iterator_definitions(class, prefix='') %} {% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %} {{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const { - m_object.m_obj = (*m_collection)[m_index]; - return m_object; + return {(*m_collection)[m_index]}; } {{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() { @@ -121,8 +120,7 @@ private: {% macro reverse_iterator_definitions(class, prefix='') %} {% with iterator_type = class.bare_type + prefix + 'CollectionReverseIterator' %} {{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const { - m_object.m_obj = (*m_collection)[m_index]; - return m_object; + return {(*m_collection)[m_index]}; } {{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() { From 7e6672e98af21eac3cd17f8e3d8b52f385eac7a0 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Fri, 1 Apr 2022 00:16:09 -0500 Subject: [PATCH 15/23] Remove read_iterators.cpp read_views.cpp again; put in unittests --- tests/CMakeLists.txt | 4 +- tests/CTestCustom.cmake | 2 - tests/read_iterators.cpp | 134 --------------------------------------- tests/read_views.cpp | 44 ------------- tests/unittest.cpp | 51 +++++++++++---- 5 files changed, 39 insertions(+), 196 deletions(-) delete mode 100644 tests/read_iterators.cpp delete mode 100644 tests/read_views.cpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 8c5059a1a..9ab80d938 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -28,7 +28,7 @@ function(CREATE_PODIO_TEST sourcefile additional_libs) ) endfunction() -set(root_dependent_tests write.cpp read.cpp read-multiple.cpp relation_range.cpp read_iterators.cpp read_views.cpp read_and_write.cpp read_and_write_associated.cpp write_timed.cpp read_timed.cpp) +set(root_dependent_tests write.cpp read.cpp read-multiple.cpp relation_range.cpp read_and_write.cpp read_and_write_associated.cpp write_timed.cpp read_timed.cpp) set(root_libs TestDataModelDict podio::podioRootIO) foreach( sourcefile ${root_dependent_tests} ) CREATE_PODIO_TEST(${sourcefile} "${root_libs}") @@ -121,8 +121,6 @@ endif() set_property(TEST read PROPERTY DEPENDS write) set_property(TEST read-multiple PROPERTY DEPENDS write) set_property(TEST read_and_write PROPERTY DEPENDS write) -set_property(TEST read_iterators PROPERTY DEPENDS write) -set_property(TEST read_views PROPERTY DEPENDS write) set_property(TEST read_timed PROPERTY DEPENDS write_timed) add_executable(check_benchmark_outputs check_benchmark_outputs.cpp) diff --git a/tests/CTestCustom.cmake b/tests/CTestCustom.cmake index f5ec23571..79a9296d8 100644 --- a/tests/CTestCustom.cmake +++ b/tests/CTestCustom.cmake @@ -20,8 +20,6 @@ if ((NOT "@FORCE_RUN_ALL_TESTS@" STREQUAL "ON") AND (NOT "@USE_SANITIZER@" STREQ check_benchmark_outputs read-multiple read-legacy-files - read_iterators - read_views write_sio read_sio diff --git a/tests/read_iterators.cpp b/tests/read_iterators.cpp deleted file mode 100644 index f1bc10545..000000000 --- a/tests/read_iterators.cpp +++ /dev/null @@ -1,134 +0,0 @@ -#include "read_test.h" -// podio specific includes -#include "podio/EventStore.h" -#include "podio/ROOTReader.h" -#include "podio/ROOTWriter.h" - -int main(){ - auto reader = podio::ROOTReader(); - auto store = podio::EventStore(); - reader.openFile("example.root"); - store.setReader(&reader); - - unsigned nEvents = reader.getEntries(); - for(unsigned i=0; i("mcparticles"); - - // STL find_if with lambda - const auto mcelectrons = std::find_if( - mcparticles.begin(), mcparticles.end(), - [](const auto& p){ return p.PDG() == 11; }); - - // ranged-based for loop - for (const auto& p: mcparticles) { - if (p.PDG() == 11) { - // do something - } - } - - // forward loop with post-increment - auto mc_first_electron_forward_post = mcparticles.end(); - for (auto p = mcparticles.begin(); p != mcparticles.end(); p++) { - if (p->PDG() == 11) { - mc_first_electron_forward_post = p; - break; - } - } - if (mc_first_electron_forward_post == mcparticles.end()) { - // no electron found - } - - // forward loop with pre-increment - auto mc_first_electron_forward_pre = mcparticles.end(); - for (auto p = mcparticles.begin(); p != mcparticles.end(); ++p) { - if (p->PDG() == 11) { - mc_first_electron_forward_pre = p; - break; - } - } - if (mc_first_electron_forward_pre == mcparticles.end()) { - // no electron found - } - - // forward loop with post-increment - auto mc_first_electron_forward_const_post = mcparticles.cend(); - for (auto p = mcparticles.cbegin(); p != mcparticles.cend(); p++) { - if (p->PDG() == 11) { - mc_first_electron_forward_const_post = p; - break; - } - } - if (mc_first_electron_forward_const_post == mcparticles.cend()) { - // no electron found - } - - // forward loop with pre-increment - auto mc_first_electron_forward_const_pre = mcparticles.cend(); - for (auto p = mcparticles.cbegin(); p != mcparticles.cend(); ++p) { - if (p->PDG() == 11) { - mc_first_electron_forward_const_pre = p; - break; - } - } - if (mc_first_electron_forward_const_pre == mcparticles.cend()) { - // no electron found - } - - // reverse loop with post-increment - auto mc_first_electron_reverse_post = mcparticles.rend(); - for (auto p = mcparticles.rbegin(); p != mcparticles.rend(); p++) { - if (p->PDG() == 11) { - mc_first_electron_reverse_post = p; - break; - } - } - if (mc_first_electron_reverse_post == mcparticles.rend()) { - // no electron found - } - - // reverse loop with pre-increment - auto mc_first_electron_reverse_pre = mcparticles.rend(); - for (auto p = mcparticles.rbegin(); p != mcparticles.rend(); ++p) { - if (p->PDG() == 11) { - mc_first_electron_reverse_pre = p; - break; - } - } - if (mc_first_electron_reverse_pre == mcparticles.rend()) { - // no electron found - } - - // reverse loop with post-increment - auto mc_first_electron_reverse_const_post = mcparticles.crend(); - for (auto p = mcparticles.crbegin(); p != mcparticles.crend(); p++) { - if (p->PDG() == 11) { - mc_first_electron_reverse_const_post = p; - break; - } - } - if (mc_first_electron_reverse_const_post == mcparticles.crend()) { - // no electron found - } - - // reverse loop with pre-increment - auto mc_first_electron_reverse_const_pre = mcparticles.crend(); - for (auto p = mcparticles.crbegin(); p != mcparticles.crend(); ++p) { - if (p->PDG() == 11) { - mc_first_electron_reverse_const_pre = p; - break; - } - } - if (mc_first_electron_reverse_const_pre == mcparticles.crend()) { - // no electron found - } - - store.clear(); - reader.endOfEvent(); - } - reader.closeFile(); - return 0; -} diff --git a/tests/read_views.cpp b/tests/read_views.cpp deleted file mode 100644 index 8506b0f05..000000000 --- a/tests/read_views.cpp +++ /dev/null @@ -1,44 +0,0 @@ -#include "read_test.h" -// podio specific includes -#include "podio/EventStore.h" -#include "podio/ROOTReader.h" -#include "podio/ROOTWriter.h" - -#ifdef __has_include -# if __has_include() -# include -# endif -# if __has_include() -# include -# endif -#endif - -int main(){ - auto reader = podio::ROOTReader(); - auto store = podio::EventStore(); - reader.openFile("example.root"); - store.setReader(&reader); - - unsigned nEvents = reader.getEntries(); - for(unsigned i=0; i("mcparticles"); - - auto v = mcparticles | std::views::reverse; - - auto is_electron = [](const auto& p){ return p.PDG() == 11; }; - for (const auto& e: mcparticles | std::views::filter(is_electron)) { - // stuff - } - #endif - - store.clear(); - reader.endOfEvent(); - } - reader.closeFile(); - return 0; -} diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 3e131a3c2..1fca29e54 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -540,31 +540,56 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { } REQUIRE(cellID == 0); + // NOTE: the following unit tests are disabled because even the mutable iterator + // does not return an lvalue on dereference: + // 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. + // // fill from const - collection.clear(); - collection.create(); - auto const_hit = ExampleHit(0x42ULL, 0., 0., 0., 0.); - //FIXME this should work but we may need a conversion constructor! + //collection.clear(); + //collection.create(); + //auto const_hit = ExampleHit(0x42ULL, 0., 0., 0., 0.); //std::fill(collection.begin(), collection.end(), const_hit); //REQUIRE(collection.begin()->cellID() == 0x42ULL); // fill from mutable - collection.clear(); - collection.create(); - auto mutable_hit = MutableExampleHit(0x42ULL, 0., 0., 0., 0.); - std::fill(collection.begin(), collection.end(), mutable_hit); - //FIXME + //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); // find in collection collection.clear(); collection.create(); - collection.create(0x42ULL, 0., 0., 0., 0.); - auto found = std::find_if(collection.begin(), collection.begin(), + 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()); - //FIXME - //REQUIRE(found->cellID() == 0x42ULL); + 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 // ranged views (C++20) From aedafdbfdf2056ae4c51d2006a1b2f20be5a9095 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 4 Apr 2022 15:43:36 -0500 Subject: [PATCH 16/23] Examples on using for_each, ranges, etc --- doc/examples.md | 36 ++++++++++++++++++++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) 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 From 01f935ae58bfccb460407a4346b4efb9ddcd0301 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 4 Apr 2022 15:44:00 -0500 Subject: [PATCH 17/23] shouldfail unit test for std::fill; added for_each to unit test --- tests/unittest.cpp | 45 +++++++++++++++++++++++++-------------------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 1fca29e54..280b0cacb 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -503,6 +503,25 @@ TEST_CASE("const correct iterators on collections", "[const-correctness]") { // objects } +TEST_CASE("Iterator dereferencing", "[!shouldfail]") { + // 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]") { // a collection to play with auto collection = ExampleHitCollection(); @@ -539,26 +558,12 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { cellID = c->cellID(); } REQUIRE(cellID == 0); - - // NOTE: the following unit tests are disabled because even the mutable iterator - // does not return an lvalue on dereference: - // 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. - // - // fill from const - //collection.clear(); - //collection.create(); - //auto const_hit = ExampleHit(0x42ULL, 0., 0., 0., 0.); - //std::fill(collection.begin(), collection.end(), const_hit); - //REQUIRE(collection.begin()->cellID() == 0x42ULL); - // 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); + // 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(); From 8291978729cc8e8968cd4dd479cbe0a4f1e989bc Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 4 Apr 2022 15:55:55 -0500 Subject: [PATCH 18/23] clang-format fixes --- tests/unittest.cpp | 79 ++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 37 deletions(-) diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 280b0cacb..ed93f7720 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -7,12 +7,12 @@ #include #include #ifdef __has_include -# if __has_include() -# include -# endif -# if __has_include() -# include -# endif + #if __has_include() + #include + #endif + #if __has_include() + #include + #endif #endif #include "catch2/catch_test_macros.hpp" @@ -527,19 +527,23 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { auto collection = ExampleHitCollection(); // bidirectional iterator traits - STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); - STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); - STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); - STATIC_REQUIRE(std::is_same_v::iterator_category, std::bidirectional_iterator_tag>); - - #ifdef __cpp_lib_ranges + STATIC_REQUIRE(std::is_same_v::iterator_category, + std::bidirectional_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, + std::bidirectional_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, + std::bidirectional_iterator_tag>); + STATIC_REQUIRE(std::is_same_v::iterator_category, + std::bidirectional_iterator_tag>); + +#ifdef __cpp_lib_ranges // bidirectional iterator concepts (C++20) STATIC_REQUIRE(std::bidirectional_iterator); STATIC_REQUIRE(std::bidirectional_iterator); STATIC_REQUIRE(std::bidirectional_iterator); STATIC_REQUIRE(std::bidirectional_iterator); STATIC_REQUIRE(std::ranges::bidirectional_range); - #endif +#endif // iterator loops auto cellID = 0; @@ -559,7 +563,7 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { } REQUIRE(cellID == 0); // std::for_each loop with reference capture - std::for_each(collection.begin(), collection.end(), [&cellID](const auto& c){ + std::for_each(collection.begin(), collection.end(), [&cellID](const auto& c) { STATIC_REQUIRE(std::is_same_v); cellID = c.cellID(); }); @@ -570,72 +574,73 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { 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; }); + 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; }); + 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; }); + 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; }); + 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; }); + 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 +#ifdef __cpp_lib_ranges // 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; }; + 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) | std::views::reverse) { + for (auto c : collection | std::views::filter(is_non_zero) | std::views::reverse) { 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::reverse(std::views::filter(collection, is_non_zero))) { + for (auto c : std::views::reverse(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) | std::views::reverse) { + for (auto c : collection | std::views::take_while(is_non_zero) | std::views::reverse) { 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) { + 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); + 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 @@ -654,7 +659,7 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { // views are references REQUIRE(collection1.begin().cellID() == 1); #endif - #endif +#endif } TEST_CASE("Subset collection basics", "[subset-colls]") { From c79589834fbf83c30342d6dca3b2ffbc27f8baed Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 1 Aug 2022 14:55:38 -0500 Subject: [PATCH 19/23] Limit iterator types to input_iterator --- python/templates/macros/iterator.jinja2 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/python/templates/macros/iterator.jinja2 b/python/templates/macros/iterator.jinja2 index 5e3b4bda8..a77fe7023 100644 --- a/python/templates/macros/iterator.jinja2 +++ b/python/templates/macros/iterator.jinja2 @@ -6,7 +6,7 @@ public: using value_type = {{ prefix }}{{ class.bare_type }}; using pointer = {{ prefix }}{{ class.bare_type }}*; using reference = {{ prefix }}{{ class.bare_type }}&; - using iterator_category = std::bidirectional_iterator_tag; + using iterator_category = std::input_iterator_tag; {{ iterator_type }}() : m_index(0), m_object(nullptr), m_collection(nullptr) {}; @@ -54,7 +54,7 @@ public: using value_type = {{prefix}}{{ class.bare_type }}; using pointer = {{prefix}}{{ class.bare_type }}*; using reference = {{prefix}}{{ class.bare_type }}&; - using iterator_category = std::bidirectional_iterator_tag; + using iterator_category = std::input_iterator_tag; {{ iterator_type }}() : m_index(0), m_object(nullptr), m_collection(nullptr) {}; From 784b902f130fb66f39723de36e14dd6c71ea655d Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 1 Aug 2022 14:58:04 -0500 Subject: [PATCH 20/23] Require input_iterator and nothing more; split off ranges tests --- tests/unittest.cpp | 52 ++++++++++++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 16 deletions(-) diff --git a/tests/unittest.cpp b/tests/unittest.cpp index ed93f7720..8ccd473e0 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -526,23 +526,36 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { // a collection to play with auto collection = ExampleHitCollection(); - // bidirectional iterator traits + // input iterator traits STATIC_REQUIRE(std::is_same_v::iterator_category, - std::bidirectional_iterator_tag>); + std::input_iterator_tag>); STATIC_REQUIRE(std::is_same_v::iterator_category, - std::bidirectional_iterator_tag>); + std::input_iterator_tag>); STATIC_REQUIRE(std::is_same_v::iterator_category, - std::bidirectional_iterator_tag>); + std::input_iterator_tag>); STATIC_REQUIRE(std::is_same_v::iterator_category, - std::bidirectional_iterator_tag>); - -#ifdef __cpp_lib_ranges - // bidirectional iterator concepts (C++20) - STATIC_REQUIRE(std::bidirectional_iterator); - STATIC_REQUIRE(std::bidirectional_iterator); - STATIC_REQUIRE(std::bidirectional_iterator); - STATIC_REQUIRE(std::bidirectional_iterator); - STATIC_REQUIRE(std::ranges::bidirectional_range); + 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 @@ -598,8 +611,15 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { auto not_found_reverse = std::find_if(collection.rbegin(), collection.rend(), [](const auto& h) { return h.cellID() == 0x1ULL; }); REQUIRE(not_found_reverse == collection.rend()); +} +TEST_CASE("Ranges (C++20)", "[ranges]") { #ifdef __cpp_lib_ranges + // 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) { @@ -612,21 +632,21 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { collection.create(0x42ULL, 1., 1., 1., 1.); // pipe syntax int count_pipe_syntax = 0; - for (auto c : collection | std::views::filter(is_non_zero) | std::views::reverse) { + 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::reverse(std::views::filter(collection, is_non_zero))) { + 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) | std::views::reverse) { + for (auto c : collection | std::views::take_while(is_non_zero)) { STATIC_REQUIRE(std::is_same_v); ++count_take_while; } From 9fed18fa16307c41361be36cb045a07ee53c3b8e Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Mon, 1 Aug 2022 15:13:14 -0500 Subject: [PATCH 21/23] Fix clang-format on unit tests --- tests/unittest.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 8ccd473e0..59923f6d7 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -527,8 +527,8 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { 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, @@ -542,8 +542,9 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { 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) From b5416758610bb5ac84fecff53e778592d693643b Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 2 Aug 2022 08:04:30 -0500 Subject: [PATCH 22/23] fix: add test case tags as suggested Co-authored-by: Thomas Madlener --- tests/unittest.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 59923f6d7..333e8e65c 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -503,7 +503,7 @@ TEST_CASE("const correct iterators on collections", "[const-correctness]") { // objects } -TEST_CASE("Iterator dereferencing", "[!shouldfail]") { +TEST_CASE("Iterator dereferencing", "[!shouldfail][iterators]") { // a collection to play with auto collection = ExampleHitCollection(); @@ -522,7 +522,7 @@ TEST_CASE("Iterator dereferencing", "[!shouldfail]") { REQUIRE(collection.begin()->cellID() == 0x42ULL); } -TEST_CASE("Iterator concepts", "[iterator-concepts]") { +TEST_CASE("Iterator concepts", "[iterator-concepts][iterators]") { // a collection to play with auto collection = ExampleHitCollection(); @@ -614,7 +614,7 @@ TEST_CASE("Iterator concepts", "[iterator-concepts]") { REQUIRE(not_found_reverse == collection.rend()); } -TEST_CASE("Ranges (C++20)", "[ranges]") { +TEST_CASE("Ranges (C++20)", "[ranges][iterators]") { #ifdef __cpp_lib_ranges // a collection to play with auto collection = ExampleHitCollection(); From ec3e58e918c463aa71d7c569cb4fbe53e61cf498 Mon Sep 17 00:00:00 2001 From: Wouter Deconinck Date: Tue, 2 Aug 2022 08:08:07 -0500 Subject: [PATCH 23/23] fix: ifdef guard entire test case as suggested Co-authored-by: Thomas Madlener --- tests/unittest.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unittest.cpp b/tests/unittest.cpp index 333e8e65c..a6a2954a3 100644 --- a/tests/unittest.cpp +++ b/tests/unittest.cpp @@ -614,8 +614,8 @@ TEST_CASE("Iterator concepts", "[iterator-concepts][iterators]") { REQUIRE(not_found_reverse == collection.rend()); } -TEST_CASE("Ranges (C++20)", "[ranges][iterators]") { #ifdef __cpp_lib_ranges +TEST_CASE("Ranges (C++20)", "[ranges][iterators]") { // a collection to play with auto collection = ExampleHitCollection(); @@ -680,8 +680,8 @@ TEST_CASE("Ranges (C++20)", "[ranges][iterators]") { // views are references REQUIRE(collection1.begin().cellID() == 1); #endif -#endif } +#endif TEST_CASE("Subset collection basics", "[subset-colls]") { auto clusterRefs = ExampleClusterCollection();