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.

Sign up for GitHub

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

Jump to bottom

Make collection iterators fulfill LegacyInputIterator and input_iterator concept #626

Draft
wants to merge 12 commits into
base: master
Choose a base branch
from
Draft

Make collection iterators fulfill LegacyInputIterator and input_iterator concept #626

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
39a21c8
fix typos
m-fila Jun 10, 2024
98fab2d
add copy construction and assignment
m-fila Jun 5, 2024
35dcd11
add multipass check references bound to the same object
m-fila Jun 10, 2024
3cb3da8
add type aliases, iterators are LegacyIterator
m-fila Jun 5, 2024
7809904
add postfix operator, iterators are LegacyInputIterators
m-fila Jun 6, 2024
1d0de7b
add value-initialization
m-fila Jun 6, 2024
5179880
use default member initializers
m-fila Jun 11, 2024
1e291b4
remove obsolete todos
m-fila Jun 11, 2024
49f2357
add missing semantic requirements checks for concepts
m-fila Jun 11, 2024
38a014f
make dereference const by not updating local object, iterators are st…
m-fila Jun 11, 2024
cc18f02
clarify lack of operator-> for std::counted_iterator adapting collect…
m-fila Jun 12, 2024
16ee2a1
fix default member initializer style
m-fila Jun 12, 2024
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
91 changes: 45 additions & 46 deletions doc/collections_as_container.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ The PODIO `Collection`s interface was designed to mimic the standard *Container*
On the implementation level most of the differences with respect to the *Container* comes from the fact that in order to satisfy the additional semantics a `Collection` doesn't directly store [user layer objects](design.md#the-user-layer). Instead, [data layer objects](design.md#the-internal-data-layer) are stored and user layer objects are constructed and returned when needed. Similarly, the `Collection` iterators operate on the user layer objects but don't expose `Collection`'s storage directly to the users. Instead, they construct and return user layer objects when needed.
In other words, a `Collection` utilizes the user layer type as a reference type instead of using plain references (`&` or `&&`) to stored data layer types.

As a consequence some of the **standard algorithms may not work** with PODIO `Collection` iterators. See [standard algorithm documentation](#collection-and-standard-algorithms) below.
As a consequence some of the **standard algorithms may not work** with PODIO `Collection` iterators. See [standard algorithm documentation](#collection-and-standard-algorithms) below.

The following tables list the compliance of a PODIO generated collection with the *Container* named requirement, stating which member types, interfaces, or concepts are fulfilled and which are not. Additionally, there are some comments explaining missing parts or pointing out differences in behaviour.

Expand All @@ -16,12 +16,12 @@ The following tables list the compliance of a PODIO generated collection with th
| Name | Type | Requirements | Fulfilled by Collection? | Comment |
|------|------|--------------|--------------------------|---------|
| `value_type` | `T` | *[Erasable](https://en.cppreference.com/w/cpp/named_req/Erasable)* | ✔️ yes | Defined as an immutable user layer object type |
| `reference` | `T&` | | ❌ no | Not defined |
| `reference` | `T&` | | ❌ no | Not defined |
| `const_reference` | `const T&` | | ❌ no | Not defined |
| `iterator` | Iterator whose `value_type` is `T` | [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) convertible to `const_iterator` | ❌ no | Defined as podio `MutableCollectionIterator`. `iterator::value_type` not defined, not [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) ([see below](#legacyforwarditerator)), not convertible to `const_iterator`|
| `const_iterator` | Constant iterator whose `value_type` is `T` | [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) | ❌ no | Defined as podio `CollectionIterator`. `const_iterator::value_type` not defined, not [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) ([see below](#legacyforwarditerator))
| `difference_type`| Signed integer | Must be the same as `std::iterator_traits::difference_type` for `iterator` and `const_iterator` | ❌ no | `std::iterator_traits::difference_type` not defined |
| `size_type` | Unsigned integer | Large enough to represent all positive values of `difference_type` | ✔️ yes | |
| `iterator` | Iterator whose `value_type` is `T` | [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) convertible to `const_iterator` | ❌ no | Defined as podio `MutableCollectionIterator`. Not [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) ([see below](#legacyforwarditerator)), not convertible to `const_iterator`|
| `const_iterator` | Constant iterator whose `value_type` is `T` | [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) | ❌ no | Defined as podio `CollectionIterator`. Not [*LegacyForwardIterator*](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) ([see below](#legacyforwarditerator))
| `difference_type`| Signed integer | Must be the same as `std::iterator_traits::difference_type` for `iterator` and `const_iterator` | ✔️ yes | |
| `size_type` | Unsigned integer | Large enough to represent all positive values of `difference_type` | ✔️ yes | |

### Container member functions and operators

Expand All @@ -31,18 +31,18 @@ The following tables list the compliance of a PODIO generated collection with th
| `C(a)` | `C` | Creates a copy of `a` | ❌ no | Not defined, non-copyable by design |
| `C(rv)` | `C` | Moves `rv` | ✔️ yes | |
| `a = b` | `C&` | Destroys or copy-assigns all elements of `a` from elements of `b` | ❌ no | Not defined, non-copyable by design |
| `a = rv` | `C&` | Destroys or move-assigns all elements of `a` from elements of `rv` | ✔️ yes | |
| `a.~C()` | `void` | Destroys all elements of `a` and frees all memory| ✔️ yes | Invalidates all handles retrieved from this collection |
| `a.begin()` | `(const_)iterator` | Iterator to the first element of `a` | ✔️ yes | |
| `a = rv` | `C&` | Destroys or move-assigns all elements of `a` from elements of `rv` | ✔️ yes | |
| `a.~C()` | `void` | Destroys all elements of `a` and frees all memory| ✔️ yes | Invalidates all handles retrieved from this collection |
| `a.begin()` | `(const_)iterator` | Iterator to the first element of `a` | ✔️ yes | |
| `a.end()` | `(const_)iterator` | Iterator to one past the last element of `a` | ✔️ yes | |
| `a.cbegin()` | `const_iterator` | Same as `const_cast<const C&>(a).begin()` | ✔️ yes | |
| `a.cend()` | `const_iterator` | Same as `const_cast<const C&>(a).end()`| ✔️ yes | |
| `a.cend()` | `const_iterator` | Same as `const_cast<const C&>(a).end()`| ✔️ yes | |
| `a == b` | Convertible to `bool` | Same as `std::equal(a.begin(), a.end(), b.begin(), b.end())`| ❌ no | Not defined |
| `a != b` | Convertible to `bool` | Same as `!(a == b)` | ❌ no | Not defined |
| `a.swap(b)` | `void` | Exchanges the values of `a` and `b` | ❌ no | Not defined |
| `swap(a,b)` | `void` | Same as `a.swap(b)` | ❌ no | `a.swap(b)` not defined |
| `a.size()` | `size_type` | Same as `std::distance(a.begin(), a.end())` | ✔️ yes | |
| `a.max_size()` | `size_type` | `b.size()` where b is the largest possible container | ✔️ yes | |
| `a.max_size()` | `size_type` | `b.size()` where `b` is the largest possible container | ✔️ yes | |
| `a.empty()` | Convertible to `bool` | Same as `a.begin() == a.end()` | ✔️ yes | |

## Collection as an *AllocatorAwareContainer*
Expand All @@ -53,9 +53,9 @@ PODIO collections don't provide a customization point for allocators and use onl

### AllocatorAwareContainer types

| Name | Requirements | Fulfilled by Collection? | Comment |
| Name | Requirements | Fulfilled by Collection? | Comment |
|------|--------------|--------------------------|---------|
| `allocator_type` | `allocator_type::value_type` same as `value_type` | ❌ no | `allocator_type` not defined |
| `allocator_type` | `allocator_type::value_type` same as `value_type` | ❌ no | `allocator_type` not defined |

### *AllocatorAwareContainer* expression and statements

Expand All @@ -70,8 +70,8 @@ In the following tables a convention from `Collection` is used: `iterator` stand

| Named requirement | `iterator` | `const_iterator` |
|-------------------|-----------------------|-----------------------------|
| [LegacyIterator](https://en.cppreference.com/w/cpp/named_req/Iterator) | ❌ no ([see below](#legacyiterator)) | ❌ no ([see below](#legacyiterator)) |
| [LegacyInputIterator](https://en.cppreference.com/w/cpp/named_req/InputIterator) | ❌ no ([see below](#legacyinputiterator)) | ❌ no ([see below](#legacyinputiterator)) |
| [LegacyIterator](https://en.cppreference.com/w/cpp/named_req/Iterator) | ✔️ yes ([see below](#legacyiterator)) | ✔️ yes ([see below](#legacyiterator)) |
| [LegacyInputIterator](https://en.cppreference.com/w/cpp/named_req/InputIterator) | ✔️ yes ([see below](#legacyinputiterator)) | ✔️ yes ([see below](#legacyinputiterator)) |
| [LegacyForwardIterator](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) | ❌ no ([see below](#legacyforwarditerator)) | ❌ no ([see below](#legacyforwarditerator)) |
| [LegacyOutputIterator](https://en.cppreference.com/w/cpp/named_req/OutputIterator) | ❌ no ([see below](#legacyoutputiterator)) | ❌ no ([see below](#legacyoutputiterator)) |
| [LegacyBidirectionalIterator](https://en.cppreference.com/w/cpp/named_req/BidirectionalIterator) | ❌ no | ❌ no |
Expand All @@ -82,9 +82,9 @@ In the following tables a convention from `Collection` is used: `iterator` stand
|---------|------------------------|------------------------------|
| `std::indirectly_readable` | ❌ no | ❌ no |
| `std::indirectly_writable` | ❌ no | ❌ no |
| `std::weakly_incrementable` | ❌ no | ❌ no |
| `std::incrementable` | ❌ no | ❌ no |
| `std::input_or_output_iterator` | ❌ no | ❌ no |
| `std::weakly_incrementable` | ✔️ yes | ✔️ yes |
| `std::incrementable` | ✔️ yes | ✔️ yes |
| `std::input_or_output_iterator` | ✔️ yes | ✔️ yes |
| `std::input_iterator` | ❌ no | ❌ no |
| `std::output_iterator` | ❌ no | ❌ no |
| `std::forward_iterator` | ❌ no | ❌ no |
Expand All @@ -96,15 +96,15 @@ In the following tables a convention from `Collection` is used: `iterator` stand

| Requirement | Fulfilled by `iterator`/`const_iterator`? | Comment |
|-------------|-------------------------------------------|---------|
| [*CopyConstructible*](https://en.cppreference.com/w/cpp/named_req/CopyConstructible) | ❌ no / ❌ no | Move constructor and copy constructor not defined |
| [*CopyAssignable*](https://en.cppreference.com/w/cpp/named_req/CopyAssignable) | ❌ no / ❌ no | Move assignment and copy assignment not defined |
| [*CopyConstructible*](https://en.cppreference.com/w/cpp/named_req/CopyConstructible) | ✔️ yes / ✔️ yes | |
| [*CopyAssignable*](https://en.cppreference.com/w/cpp/named_req/CopyAssignable) | ✔️ yes / ✔️ yes | |
| [*Destructible*](https://en.cppreference.com/w/cpp/named_req/Destructible) | ✔️ yes / ✔️ yes | |
| [*Swappable*](https://en.cppreference.com/w/cpp/named_req/Swappable) | ❌ no / ❌ no | |
| `std::iterator_traits::value_type` (Until C++20 ) | ❌ no / ❌ no | Not defined |
| `std::iterator_traits::difference_type` | ❌ no / ❌ no | Not defined |
| `std::iterator_traits::reference` | ❌ no / ❌ no | Not defined |
| `std::iterator_traits::pointer` | ❌ no / ❌ no | Not defined |
| `std::iterator_traits::iterator_category` | ❌ no / ❌ no | Not defined |
| [*Swappable*](https://en.cppreference.com/w/cpp/named_req/Swappable) | ✔️ yes / ✔️ yes | |
| `std::iterator_traits::value_type` (Until C++20 ) | ✔️ yes / ✔️ yes | |
| `std::iterator_traits::difference_type` | ✔️ yes / ✔️ yes | |
| `std::iterator_traits::reference` | ✔️ yes / ✔️ yes | |
| `std::iterator_traits::pointer` | ✔️ yes / ✔️ yes | |
| `std::iterator_traits::iterator_category` | ✔️ yes / ✔️ yes | |

| Expression | Return type | Semantics | Fulfilled by `iterator`/`const_iterator`? | Comment |
|------------|-------------|-----------|-------------------------------------------|---------|
Expand All @@ -115,49 +115,48 @@ In the following tables a convention from `Collection` is used: `iterator` stand

| Requirement | Fulfilled by `iterator`/`const_iterator`? | Comment |
|-------------|-------------------------------------------|---------|
| [*LegacyIterator*](https://en.cppreference.com/w/cpp/named_req/Iterator) | ❌ no / ❌ no | [See above](#legacyiterator) |
| [*LegacyIterator*](https://en.cppreference.com/w/cpp/named_req/Iterator) | ✔️ yes / ✔️ yes | [See above](#legacyiterator) |
| [*EqualityComparable*](https://en.cppreference.com/w/cpp/named_req/EqualityComparable) | ✔️ yes / ✔️ yes | |

| Expression | Return type | Semantics | Fulfilled by `iterator`/`const_iterator`? | Comment |
|------------|-------------|-----------|-------------------------------------------|---------|
| `i != j` | Contextually convertible to `bool` | Same as `!(i==j)` | ✔️ yes / ✔️ yes | |
| `*i` | `reference`, convertible to `value_type` | | ❌ no / ❌ no | `reference` and `value_type` not defined |
| `i != j` | Contextually convertible to `bool` | Same as `!(i==j)` | ✔️ yes / ✔️ yes | |
| `*i` | `reference`, convertible to `value_type` | | ✔️ yes / ✔️ yes | |
| `i->m` | | Same as `(*i).m` | ✔️ yes / ✔️ yes | |
| `++r` | `It&` | | ✔️ yes / ✔️ yes | |
| `(void)r++` | | Same as `(void)++r` | ❌ no / ❌ no | Post-increment not defined |
| `*r++` | Convertible to `value_type` | Same as `value_type x = *r; ++r; return x;` | ❌ no / ❌ no | Post-increment and `value_type` not defined |
| `(void)r++` | | Same as `(void)++r` | ✔️ yes / ✔️ yes | |
| `*r++` | Convertible to `value_type` | Same as `value_type x = *r; ++r; return x;` | ✔️ yes / ✔️ yes | |

### LegacyForwardIterator

In addition to the *LegacyForwardIterator* the C++ standard specifies also the *mutable LegacyForwardIterator*, which is both *LegacyForwardIterator* and *LegacyOutputIterator*. The term **mutable** used in this context doesn't imply mutability in the sense used in the PODIO.


| Requirement | Fulfilled by `iterator`/`const_iterator`? | Comment |
|-------------|-------------------------------------------|---------|
| [*LegacyInputIterator*](https://en.cppreference.com/w/cpp/named_req/InputIterator) | ❌ no / ❌ no | [See above](#legacyinputiterator)|
| [*DefaultConstructible*](https://en.cppreference.com/w/cpp/named_req/DefaultConstructible) | ❌ no / ❌ no | Value initialization not defined |
| If *mutable* iterator then `reference` same as `value_type&` or `value_type&&`, otherwise same as `const value_type&` or `const value_type&&` | ❌ no / ❌ no | `reference` and `value_type` not defined |
| [Multipass guarantee](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) | ❌ no / ❌ no | Copy constructor not defined |
| [Singular iterators](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) | ❌ no / ❌ no | Value initialization not defined |
| [*LegacyInputIterator*](https://en.cppreference.com/w/cpp/named_req/InputIterator) | ✔️ yes / ✔️ yes | [See above](#legacyinputiterator)|
| [*DefaultConstructible*](https://en.cppreference.com/w/cpp/named_req/DefaultConstructible) | ✔️ yes / ✔️ yes | |
| If *mutable* iterator then `reference` same as `value_type&` or `value_type&&`, otherwise same as `const value_type&` or `const value_type&&` | ❌ no / ❌ no | `reference` type is not a reference (`&` or `&&`) |
| [Multipass guarantee](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) | ❌ no / ❌ no | References from dereferencing equal iterators aren't bound to the same object |
| [Singular iterators](https://en.cppreference.com/w/cpp/named_req/ForwardIterator) | ✔️ yes / ✔️ yes | |

| Expression | Return type | Semantics | Fulfilled by `iterator`/`const_iterator`? | Comment |
|------------|-------------|-----------|-------------------------------------------|---------|
| `i++` | `It` | Same as `It ip = i; ++i; return ip;` | ❌ no / ❌ no | Post-increment not defined |
| `*i++` | `reference` | | ❌ no / ❌ no | Post-increment and `reference` not defined |
| `i++` | `It` | Same as `It ip = i; ++i; return ip;` | ✔️ yes / ✔️ yes | |
| `*i++` | `reference` | | ✔️ yes / ✔️ yes | |

### LegacyOutputIterator

| Requirement | Fulfilled by `iterator`/`const_iterator`? | Comment |
|-------------|-------------------------------------------|---------|
| [*LegacyIterator*](https://en.cppreference.com/w/cpp/named_req/Iterator) | ❌ no / ❌ no | [See above](#legacyiterator) |
| [*LegacyIterator*](https://en.cppreference.com/w/cpp/named_req/Iterator) | ✔️ yes / ✔️ yes | [See above](#legacyiterator) |
| Is pointer type or class type | ✔️ yes / ✔️ yes | |

| Expression | Return type | Semantics | Fulfilled by `iterator`/`const_iterator`? | Comment |
|------------|-------------|-----------|-------------------------------------------|---------|
| `*r = o` | | | ❗ attention / ❗ attention | Defined but an assignment doesn't modify objects inside collection |
| `++r` | `It&` | | ✔️ yes / ✔️ yes | |
| `r++` | Convertible to `const It&` | Same as `It temp = r; ++r; return temp;` | ❌ no / ❌ no | Post-increment not defined |
| `*r++ = o` | | Same as `*r = o; ++r;`| ❌ no / ❌ no | Post-increment not defined |
| `++r` | `It&` | | ✔️ yes / ✔️ yes | |
| `r++` | Convertible to `const It&` | Same as `It temp = r; ++r; return temp;` | ✔️ yes / ✔️ yes | |
| `*r++ = o` | | Same as `*r = o; ++r;`| ❗ attention / ❗ attention | Defined but an assignment doesn't modify objects inside collection |

## Collection iterators and standard iterator adaptors

Expand All @@ -167,9 +166,9 @@ In addition to the *LegacyForwardIterator* the C++ standard specifies also the *
| `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 |
| `std::const_iterator` | ❌ no | `iterator` and `const_iterator` not *LegacyInputIterator* or `std::input_iterator` |
| `std::move_iterator` | ❌ no | `iterator` and `const_iterator` not *LegacyInputIterator* or `std::input_iterator` |
| `std::counted_iterator` | ❌ no | `iterator` and `const_iterator` not `std::input_or_output_iterator` |
| `std::const_iterator` (C++23) | ❌ no | C++23 not supported yet |
| `std::move_iterator` | ✔️ yes | Limited usefulness since dereference returns `reference` not rvalue (`&&`) |
| `std::counted_iterator` | ✔️ yes | |


## Collection and standard algorithms
Expand Down
29 changes: 23 additions & 6 deletions python/templates/macros/iterator.jinja2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,20 @@
{% set ptr_init = 'podio::utils::MaybeSharedPtr<' + class.bare_type +'Obj>{nullptr}' %}
class {{ iterator_type }} {
public:
using value_type = {{ class.bare_type }};
using difference_type = ptrdiff_t;
using reference = {{ prefix }}{{ class.bare_type }};
using pointer = {{ prefix }}{{ 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({{ ptr_init }}), m_collection(collection) {}
{{ iterator_type }}(): {{ iterator_type }}(0, nullptr) {}
m-fila marked this conversation as resolved.
Show resolved Hide resolved

{{ iterator_type }}(const {{ iterator_type }}&) = delete;
{{ iterator_type }}& operator=(const {{ iterator_type }}&) = delete;
{{ iterator_type }}(const {{ iterator_type }}&) = default;
{{ iterator_type }}({{ iterator_type }}&&) = default;
{{ iterator_type }}& operator=(const {{ iterator_type }}&) = default;
{{ iterator_type }}& operator=({{iterator_type}}&&) = default;
m-fila marked this conversation as resolved.
Show resolved Hide resolved
~{{ iterator_type }}() = default;

bool operator!=(const {{ iterator_type}}& x) const {
return m_index != x.m_index; // TODO: may not be complete
Expand All @@ -16,9 +26,10 @@ public:
return m_index == x.m_index; // TODO: may not be complete
}

{{ prefix }}{{ class.bare_type }} operator*();
{{ prefix }}{{ class.bare_type }}* operator->();
reference operator*();
pointer operator->();
{{ iterator_type }}& operator++();
{{ iterator_type }} operator++(int);

private:
size_t m_index;
Expand All @@ -32,12 +43,12 @@ private:
{% macro iterator_definitions(class, prefix='') %}
{% with iterator_type = class.bare_type + prefix + 'CollectionIterator' %}
{% set ptr_type = 'podio::utils::MaybeSharedPtr<' + class.bare_type +'Obj>' %}
{{ prefix }}{{ class.bare_type }} {{ iterator_type }}::operator*() {
{{ iterator_type }}::reference {{ iterator_type }}::operator*() {
m_object.m_obj = {{ ptr_type }}((*m_collection)[m_index]);
return m_object;
}

{{ prefix }}{{ class.bare_type }}* {{ iterator_type }}::operator->() {
{{ iterator_type }}::pointer {{ iterator_type }}::operator->() {
m_object.m_obj = {{ ptr_type }}((*m_collection)[m_index]);
return &m_object;
}
Expand All @@ -47,5 +58,11 @@ private:
return *this;
}

{{ iterator_type }} {{ iterator_type }}::operator++(int) {
auto copy = *this;
++m_index;
return copy;
}

{% endwith %}
{% endmacro %}
Loading