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

Make collection iterators fulfill LegacyInputIterator and input_iterator concept #626

Merged
merged 24 commits into from
Dec 19, 2024

Conversation

m-fila
Copy link
Contributor

@m-fila m-fila commented Jun 11, 2024

BEGINRELEASENOTES

  • Added missing operations and type aliases so the collection iterators fulfill LegacyInputIterator requirement and std::input_iterator concept, and the collections fulfill std::input_range concept. Thanks to these the collections are compatible with standard algorithms and standard range algorithms such as std::find, std::count, std::all_of

ENDRELEASENOTES

Fixes: #150, fixes #272
Conflicts: #273

Implement missing features so CollectionIterators and MutableCollectionIterators 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:

  • LegacyForwardIterator: dereference should return a reference, but in podio iterators are 'proxy iterators' returning user layer objects as a proxy for reference
  • LegacyForwardItertor: dereferencing two iterators that are equal should give two references bound to the same object, but in podio the returned 'proxies' are different objects
  • 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.
  • LegacyOutputIterator/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 be std::ranges::input_range. This PR implements missing features to achieve this.

List of changes:

Named requirements:

  • LegacyIterator:
    • copy constructor and assignment
    • type aliases
  • LegacyInputIterator:
    • postfix increment
  • LegacForwardIterator:
    • value-initialization
    • ❌ dereference return reference not proxy
    • ❌ dereferencing equal iterators returns references to the same object
  • OutputIterator:
    • ❌ assignment should modify collection
  • iterator_category is std::input_iterator_tag
  • check selected algorithms operating on LegacyInputIterators

Concepts:

  • semantics checks for concepts
  • std::input_or_output_iterator
    • same as LegacyIterator but copyability not required
  • std::input_iterator
    • std::indirectly_readable and std::readable (can dereference both iterator and const iterator)
  • std::forward_iterator
    • syntax: constrains (except iterator_concept to be derived fromstd::forward_iterator_tag)
    • ❌ semantic: pointers obtained from iterator should remain valid as long as range is valid (no-stashing)
  • iterator_concept is std::input_iterator_tag
  • check range concepts
  • check selected range algorithms constrained to std::input_iterator

@m-fila m-fila force-pushed the upgrade_iterators branch from 7117c3f to 1d0de7b Compare June 11, 2024 17:31
Copy link
Collaborator

@tmadlener tmadlener left a 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).

python/templates/macros/iterator.jinja2 Show resolved Hide resolved
python/templates/macros/iterator.jinja2 Outdated Show resolved Hide resolved
@Zehvogel
Copy link
Contributor

can you also take a look at podio::RelationRange? :)

@m-fila
Copy link
Contributor Author

m-fila commented Sep 25, 2024

Thanks for pointing out, I'll check it out later

@tmadlener
Copy link
Collaborator

Stupid question:

LegacyForwardItertor: dereferencing two iterators that are equal should give two references bound to the same object, but in podio the returned 'proxies' are different objects

What are the semantic requirements for this? operator== returns true when comparing them? Or do they literally have to be a reference to the same object, i.e. the address of the referenced object has to be the same?

If the former is the case, than this should be doable, I think. Because our operator== compares the handled Obj.

@m-fila
Copy link
Contributor Author

m-fila commented Dec 12, 2024

What are the semantic requirements for this? operator== returns true when comparing them? Or do they literally have to be a reference to the same object, i.e. the address of the referenced object has to be the same?

If the former is the case, than this should be doable, I think. Because our operator== compares the handled Obj.

Unfortunately it's the second. One of the previous points already states that *it must be a reference (as in & or &&), then

If i and j are both dereferenceable, then i == j if and only if *i and *j are bound to the same object.


I think this is ready.
We can have LegacyInputIterator and std::input_iterator without big changes.

  • LegacyForwardIterator seems to be fundamentally not compatible with the semantics we have,
  • LegacyOutputIterator and std::output_iterator would require to change the nature of assignment in datatypes - which is a big change that I don't really want to force here.
  • std::forward_iterator is almost there. If this gets merged then I could separate discussion whether we can get it with some disclaimers.

@m-fila m-fila marked this pull request as ready for review December 12, 2024 07:53
Copy link
Collaborator

@tmadlener tmadlener left a 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.

doc/collections_as_container.md Outdated Show resolved Hide resolved
doc/collections_as_container.md Outdated Show resolved Hide resolved
tests/unittests/std_interoperability.cpp Show resolved Hide resolved
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.};
Copy link
Collaborator

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)?

Copy link
Contributor Author

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

Copy link
Collaborator

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.

tests/unittests/std_interoperability.cpp Outdated Show resolved Hide resolved
tests/unittests/std_interoperability.cpp Outdated Show resolved Hide resolved
@tmadlener tmadlener merged commit 3f8bd29 into AIDASoft:master Dec 19, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants