Deleted iterator copy constructor/assignment and finding

[wdconinc]

I suspect there are good reasons to delete the iterators in python/template/macros/iterator/iterator.jinja2, but it makes searching through a collection a bit more cumbersome at times. I am wondering if there are ways in which some of the choices on iterator copy constructors and assignment operators deletion can be relaxed, or am I missing something I should be using instead?

The following are attempts at finding the first electron in an MCParticlesCollection. I am using gcc-11 with -std=gnu++20.

Using find_if to get an iterator to the first electron

    const auto mc_first_electron = std::find_if(
      mcparts.begin(),
      mcparts.end(),
      [](const auto& p){ return p.getPDG() == 11; });

This fails because MCParticleCollectionIterator has a deleted copy constructor which is required (likely) to return a copy of the internal iterator inside std::find_if.

Using a simple (old-style) iterator loop

If we can't copy an iterator, then let's avoid the copy.

    auto mc_first_electron = mcparts.end();
    for (auto p = mcparts.begin(); p != mcparts.end(); p++) {
      if (p->getPDG() == 11) {
        mc_first_electron = p;
        break;
      }
    }

This fails because iterator post-increment is not implemented. Ok, easy enough to change (and arguably better though unlikely that an iterator takes up so much memory this matters here; still, it is good to instill best practices).

    auto mc_first_electron = mcparts.end();
    for (auto p = mcparts.begin(); p != mcparts.end(); ++p) {
      if (p->getPDG() == 11) {
        mc_first_electron = p;
        break;
      }
    }

This still fails but now because we cannot assign p to mc_first_electron since the assignment operator for iterators is deleted. Ok, we can avoid this assignment too, by avoiding a locally scoped iterator.

    auto mc_first_electron = mcparts.begin();
    for (; mc_first_electron != mcparts.end(); ++mc_first_electron) {
      if (mc_first_electron->getPDG() == 11) {
        break;
      }
    }

Ah, we have something that compiles.

Checking that we actually found something

After the search, we want to make sure we found something (and return if not).

    if (mc_first_electron == mcparts.end()) {
      debug() << "No electron found" << endmsg;
      return StatusCode::FAILURE;
    }

This now fails again because operator== isn't defined for iterators. We already used operator!= above, so we end up with this:

    if (!(mc_first_electron != mcparts.end())) {
      debug() << "No electron found" << endmsg;
      return StatusCode::FAILURE;
    }

Ultimately, in a c++20 world,...

When I started off, I had hopes of being able to use the following compact syntax:

    auto is_electron = [](const auto& p){ return p.getPDG() == 11; };
    for (const auto& e: mcparts | std::views::filter(is_electron)) {
      // stuff
    }

This does not compile (unsurprisingly) due to the missing operator| on collections. Implementing this doesn't seem too onerous but I think I'd need to understand first what the design decisions were for the deleted iterators and assignment operators.

[wdconinc]

You might also ask why we don't just use

    for (const auto& p: mcparts) {
      if (p.getPDG() == 11) {
        // stuff
      }
    }

The reason is that we need to do a few more of these searches, and have to combine the results. Nesting deeper and deeper just makes the code more cumbersome.

[wdconinc]

Finally, the following quote from the link on c++20 range views above is interesting:

For a std::forward_iterator, the requirements are just slightly tighter:

• The iterator must be copyable.
• It must be equality-comparable with other iterators of the same container.
• The postincrement operator must return a copy of the iterator before modification.

Those three are of course exactly the ones that are missing :-)

[tmadlener]

Thanks for the nice summary and also pointers to the c++ standard requirements. Let me try and add a few podio details to the discussion.

• I think collections having a forward iterator is actually a quite important feature and from a technical point of view I don't see anything that prohibits having this. (The fact that it is missing at the moment, can mostly be attributed to a lack of manpower / time).
• For const collections a forward iterator is also probably as far as we can go as there the order of the elements in the collections is crucially depended on for I/O purposes. So after we have read a collection we cannot easily reshuffle it, without breaking a lot of things.
• For non-const collections (that haven't been written yet) there might be a chance to even have a random access iterator, but that would require a bit more thought and I am not entirely sure if it is possible in the end, as there could be some subtleties.

If you want to have a go at this, we would be very happy to accept a PR for this. I am working on a bit of documentation for the template things, so that it should become possible for others, not as deeply into the whole thing as me to work on the templates.

Just for completeness, we seem to have this on our list already (somehow): #150 but having another request for this might push this a bit further up :)