Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure iterators satisfy input_iterator concept for views and ranges #273

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
383efc6
input_iterator type and unit tests
wdconinc Mar 28, 2022
540eae4
Make checks happy by using all variables
wdconinc Mar 28, 2022
db113f8
Haha, get past the sanitzer checks
wdconinc Mar 28, 2022
dc6c782
Add reverse iterators; split tests into iterators and views
wdconinc Mar 29, 2022
f74e64e
Add prefix to iterator types; change to bidirectional_iterator_tag
wdconinc Mar 30, 2022
d2c783b
Add iterator default constructor (index 0, nullptr)
wdconinc Mar 30, 2022
07fc73a
No getter for PDG in read_views test
wdconinc Mar 30, 2022
3ae4087
Const dereference with mutable m_object
wdconinc Mar 30, 2022
f83d7bc
Full(er) set of unit tests (incl c++23 zip)
wdconinc Mar 30, 2022
853423b
Single line import of iterator_definitions
wdconinc Mar 30, 2022
a71f643
Remove reserve for sized_range since not available for podio collection
wdconinc Mar 30, 2022
74e4691
Suggested style change
wdconinc Mar 30, 2022
4b5cf9f
static_assert on iterator_traits for bidirectional_iterator_tag
wdconinc Mar 30, 2022
d18b1d6
dereference operator* const to copy without mutable inside iterator
wdconinc Mar 30, 2022
7e6672e
Remove read_iterators.cpp read_views.cpp again; put in unittests
wdconinc Apr 1, 2022
aedafdb
Examples on using for_each, ranges, etc
wdconinc Apr 4, 2022
01f935a
shouldfail unit test for std::fill; added for_each to unit test
wdconinc Apr 4, 2022
8291978
clang-format fixes
wdconinc Apr 4, 2022
c795898
Limit iterator types to input_iterator
wdconinc Aug 1, 2022
784b902
Require input_iterator and nothing more; split off ranges tests
wdconinc Aug 1, 2022
9fed18f
Fix clang-format on unit tests
wdconinc Aug 1, 2022
b541675
fix: add test case tags as suggested
wdconinc Aug 2, 2022
ec3e58e
fix: ifdef guard entire test case as suggested
wdconinc Aug 2, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ endif()
# ``-DCMAKE_CXX_STANDARD=<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")
tmadlener marked this conversation as resolved.
Show resolved Hide resolved
message(FATAL_ERROR "Unsupported C++ standard: ${CMAKE_CXX_STANDARD}")
endif()

Expand Down
36 changes: 34 additions & 2 deletions doc/examples.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,22 +65,54 @@ 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) {
std::cout << i->energy() << std::endl;
}
```

and via direct object access:
Via direct object access:

```cpp
for(int i = 0, end = hits.size(), i != end, ++i){
std::cout << hit[i].energy() << std::endl;
}
```

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
Expand Down
6 changes: 5 additions & 1 deletion python/templates/Collection.cc.jinja2
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{% import "macros/utils.jinja2" as utils %}
{% import "macros/collections.jinja2" as macros %}
{% from "macros/iterator.jinja2" import iterator_definitions %}
{% from "macros/iterator.jinja2" import iterator_definitions, reverse_iterator_definitions %}
// AUTOMATICALLY GENERATED FILE - DO NOT EDIT

#include "{{ incfolder }}{{ class.bare_type }}Collection.h"
Expand Down Expand Up @@ -152,6 +152,10 @@ podio::CollectionBuffers {{ collection_type }}::getBuffers() {

{{ iterator_definitions(class, prefix='Mutable' ) }}

{{ reverse_iterator_definitions(class) }}

{{ reverse_iterator_definitions(class, prefix='Mutable' ) }}

{{ macros.ostream_operator(class, Members, OneToOneRelations, OneToManyRelations, VectorMembers, use_get_syntax, ostream_collection_settings) }}

{{ utils.namespace_close(class.namespace) }}
49 changes: 45 additions & 4 deletions python/templates/Collection.h.jinja2
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -26,13 +27,18 @@
#include <ostream>
#include <mutex>
#include <memory>
#if __has_include(<ranges>)
# include <ranges>
#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.
Expand All @@ -41,8 +47,19 @@ class {{ class.bare_type }}Collection : public podio::CollectionBase {
public:
using const_iterator = {{ class.bare_type }}CollectionIterator;
using iterator = {{ class.bare_type }}MutableCollectionIterator;
using const_reverse_iterator = {{ class.bare_type }}CollectionReverseIterator;
using reverse_iterator = {{ class.bare_type }}MutableCollectionReverseIterator;

{{ class.bare_type }}Collection();
#if __cpp_lib_ranges
{{ class.bare_type }}Collection(std::ranges::input_range auto&& range)
requires(std::convertible_to<std::ranges::range_value_t<decltype(range)>, {{ class.bare_type }}>)
: m_isValid(false), m_isPrepared(false), m_isSubsetColl(false), m_collectionID(0), m_storage() {
for (auto&& item : range) {
push_back(item);
}
};
#endif
// This is a move-only type
{{ class.bare_type }}Collection(const {{ class.bare_type}}Collection& ) = delete;
{{ class.bare_type }}Collection& operator=(const {{ class.bare_type}}Collection& ) = delete;
Expand Down Expand Up @@ -120,18 +137,42 @@ public:
}

// support for the iterator protocol
iterator begin() {
iterator begin() noexcept {
return iterator(0, &m_storage.entries);
}
const_iterator begin() const {
const_iterator cbegin() const noexcept {
return const_iterator(0, &m_storage.entries);
}
iterator end() {
const_iterator begin() const noexcept {
return const_iterator(0, &m_storage.entries);
}
reverse_iterator rbegin() noexcept {
return reverse_iterator(m_storage.entries.size() - 1, &m_storage.entries);
}
const_reverse_iterator crbegin() const noexcept {
return const_reverse_iterator(m_storage.entries.size() - 1, &m_storage.entries);
}
const_reverse_iterator rbegin() const noexcept {
return const_reverse_iterator(m_storage.entries.size() - 1, &m_storage.entries);
}
iterator end() noexcept {
return iterator(m_storage.entries.size(), &m_storage.entries);
}
const_iterator end() const {
const_iterator cend() const noexcept {
return const_iterator(m_storage.entries.size(), &m_storage.entries);
}
const_iterator end() const noexcept {
return const_iterator(m_storage.entries.size(), &m_storage.entries);
}
reverse_iterator rend() noexcept {
return reverse_iterator(-1, &m_storage.entries);
}
const_reverse_iterator crend() const noexcept {
return const_reverse_iterator(-1, &m_storage.entries);
}
const_reverse_iterator rend() const noexcept {
return const_reverse_iterator(-1, &m_storage.entries);
}

{% for member in Members %}
template<size_t arraysize>
Expand Down
1 change: 1 addition & 0 deletions python/templates/MutableObject.h.jinja2
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions python/templates/Object.h.jinja2
Original file line number Diff line number Diff line change
Expand Up @@ -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) }}
Expand Down
110 changes: 103 additions & 7 deletions python/templates/macros/iterator.jinja2
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,41 @@
{% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %}
class {{ iterator_type }} {
public:
{{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection) : m_index(index), m_object(nullptr), m_collection(collection) {}
using difference_type = std::ptrdiff_t;
using value_type = {{ prefix }}{{ class.bare_type }};
using pointer = {{ prefix }}{{ class.bare_type }}*;
using reference = {{ prefix }}{{ class.bare_type }}&;
using iterator_category = std::input_iterator_tag;

{{ iterator_type }}(const {{ iterator_type }}&) = delete;
{{ iterator_type }}& operator=(const {{ iterator_type }}&) = delete;
{{ iterator_type }}()
: m_index(0), m_object(nullptr), m_collection(nullptr) {};
{{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection)
: m_index(index), m_object(nullptr), m_collection(collection) {};

{{ iterator_type }}(const {{ iterator_type }}&) = default;
{{ iterator_type }}& operator=(const {{ iterator_type }}&) = default;

bool operator!=(const {{ iterator_type}}& x) const {
return m_index != x.m_index; // TODO: may not be complete
}
bool operator==(const {{ iterator_type}}& x) const {
return !operator!=(x);
}

{{ prefix }}{{ class.bare_type }} operator*();
{{ prefix }}{{ class.bare_type }} operator*() const;
{{ prefix }}{{ class.bare_type }}* operator->();
{{ iterator_type }}& operator++();
{{ iterator_type }} operator++(int) {
{{ iterator_type }} temp = *this;
++*this;
return temp;
};
{{ iterator_type }}& operator--();
{{ iterator_type }} operator--(int) {
{{ iterator_type }} temp = *this;
--*this;
return temp;
};

private:
size_t m_index;
Expand All @@ -23,12 +46,58 @@ private:
{% endwith %}
{% endmacro %}

{% macro reverse_iterator_declaration(class, prefix='') %}
{% with iterator_type = class.bare_type + prefix + 'CollectionReverseIterator' %}
class {{ iterator_type }} {
public:
using difference_type = std::ptrdiff_t;
using value_type = {{prefix}}{{ class.bare_type }};
using pointer = {{prefix}}{{ class.bare_type }}*;
using reference = {{prefix}}{{ class.bare_type }}&;
using iterator_category = std::input_iterator_tag;

{{ iterator_type }}()
: m_index(0), m_object(nullptr), m_collection(nullptr) {};
{{ iterator_type }}(size_t index, const {{ class.bare_type }}ObjPointerContainer* collection)
: m_index(index), m_object(nullptr), m_collection(collection) {};

{{ iterator_type }}(const {{ iterator_type }}&) = default;
{{ iterator_type }}& operator=(const {{ iterator_type }}&) = default;

bool operator!=(const {{ iterator_type }}& x) const {
return m_index != x.m_index; // TODO: may not be complete
}
bool operator==(const {{ iterator_type }}& x) const {
return !operator!=(x);
}

{{ prefix }}{{ class.bare_type }} operator*() const;
{{ prefix }}{{ class.bare_type }}* operator->();
{{ iterator_type }}& operator++();
{{ iterator_type }} operator++(int) {
{{ iterator_type }} temp = *this;
++*this;
return temp;
};
{{ iterator_type }}& operator--();
{{ iterator_type }} operator--(int) {
{{ iterator_type }} temp = *this;
--*this;
return temp;
};

private:
size_t m_index;
{{ prefix }}{{ class.bare_type }} m_object;
const {{ class.bare_type }}ObjPointerContainer* m_collection;
};
{% endwith %}
{% endmacro %}

{% macro iterator_definitions(class, prefix='') %}
{% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %}
{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() {
m_object.m_obj = (*m_collection)[m_index];
return m_object;
{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const {
return {(*m_collection)[m_index]};
}

{{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() {
Expand All @@ -41,5 +110,32 @@ private:
return *this;
}

{{ iterator_type }}& {{ iterator_type }}::operator--() {
--m_index;
return *this;
}
{% endwith %}
{% endmacro %}

{% macro reverse_iterator_definitions(class, prefix='') %}
{% with iterator_type = class.bare_type + prefix + 'CollectionReverseIterator' %}
{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() const {
return {(*m_collection)[m_index]};
}

{{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() {
m_object.m_obj = (*m_collection)[m_index];
return &m_object;
}

{{ iterator_type }}& {{ iterator_type }}::operator++() {
--m_index;
return *this;
}

{{ iterator_type }}& {{ iterator_type }}::operator--() {
++m_index;
return *this;
}
{% endwith %}
{% endmacro %}
Loading