-
Notifications
You must be signed in to change notification settings - Fork 60
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
Make collection iterators fulfill LegacyInputIterator and input_iterator concept #626
Conversation
7117c3f
to
1d0de7b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not fully done yet, but I think it would be nice to also document which (classes of) algorithms will be enabled by this (if any).
can you also take a look at |
Thanks for pointing out, I'll check it out later |
16ee2a1
to
690d24c
Compare
Stupid question:
What are the semantic requirements for this? If the former is the case, than this should be doable, I think. Because our |
Unfortunately it's the second. One of the previous points already states that
I think this is ready.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for all the work. I mainly have a few minor questions, resp. some (hopefully small) requests for some potential re-organization.
auto coll = CollectionType{}; | ||
auto item = coll.create(13ull, 0., 0., 0., 0.); | ||
REQUIRE(coll.cbegin()->cellID() == 13ull); | ||
auto new_item = CollectionType::value_type::mutable_type{42ull, 0., 0., 0., 0.}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like we should consider adding mutable_type
directly to the collection as well (I don't think we have yet)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have it in link collections but not in the datatype collections. I guess then we should also think if there should be one in user data collection to keep the api somehow similar.
But I think this could be done in a separate PR as it's not touching our compliance to iterator specification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely a separate PR.
cbb6c3c
to
68c4dbc
Compare
…d::input_iterators
Co-authored-by: Thomas Madlener <[email protected]>
2317b2d
to
3bac2d7
Compare
BEGINRELEASENOTES
std::input_iterator
concept, and the collections fulfillstd::input_range
concept. Thanks to these the collections are compatible with standard algorithms and standard range algorithms such asstd::find
,std::count
,std::all_of
ENDRELEASENOTES
Fixes: #150, fixes #272
Conflicts: #273
Implement missing features so
CollectionIterators
andMutableCollectionIterators
fulfill the requirements imposed on the C++ iterators (#598). It's expected than not all requirements can be fulfilled as some are in direct conflict with podio design:std::forward_iterator
: pointers and references obtained from an iterator should be valid as range the range is valid, but in podio the iterators behave like 'stashing iterators' -> pointers point to iterator member variable limiting their validity to the lifetime of the iterator.output_iterator
: datatype assignment replaces the internal object handle instead of modifying the handle properties.As a consequence the iterators can the podio iterators can be at most LegacyInputIterator and
std::input_iterator
, and collections bestd::ranges::input_range
. This PR implements missing features to achieve this.List of changes:
Named requirements:
iterator_category
isstd::input_iterator_tag
Concepts:
std::input_or_output_iterator
std::input_iterator
std::indirectly_readable
andstd::readable
(can dereference bothiterator
andconst iterator
)std::forward_iterator
iterator_concept
to be derived fromstd::forward_iterator_tag
)iterator_concept
isstd::input_iterator_tag
std::input_iterator