Make link collection iterators behave consistently with generated collection iterators

[tmadlener]

original outset of this PR: non-const collections give access to mutable handles via operator[] and at(). Hence, they should also model ranges over mutable handles. The fix is straight forward: Make sure the iterators report the correct value_type.

See the discussion below for why we now make the link collections behave the same as the generated ones.

BEGINRELEASENOTES

• Make the LinkCollectionIteratorT::value_type an immutable Link always for consistency with the behavior of generated collection iterators.

ENDRELEASENOTES

@m-fila am I missing something obvious here?

[m-fila]

We had value_type in iterators defined to immutable type to fulfill some of the "Container" requirements.

We already don't fulfill "Container" for a number of reasons, so it should be fine to break it slightly more as long as we don't break requirements for iterators and ranges (which aren't affected by it)

[tmadlener]

Right, so some nuances still here. We don't really change any behavior that allows to break things that weren't already broken before, I think.

Just to confirm, the main thing that now is possible that wasn't before is this:

ExampleClusterCollection coll{};
for (auto& elem : coll) {
  coll.setEnergy(42);  // not possible before because elem would not have a setter
}

One would have to do this (which is also already possible now)

for (size_t i = 0; i < coll.size(); ++i) {
  coll[i].setEnergy(42);  // possible because operator[] returns a mutable handle on non-const collections
}

What is broken regardless of these changes is assigning a new handle to the element, I think. Or does that also change with this change? (Do we have a test for that that I simply missed?).

Anyhow, the main reason I wanted to have this feature, is because it makes it behave the same as the LinkCollection, where we do:

podio/include/podio/detail/LinkCollectionIterator.h

Lines 11 to 15 in 35e65ce

	using LinkType = LinkT<FromT, ToT, Mutable>;
	using LinkObjT = LinkObj<FromT, ToT>;
	public:
	using value_type = LinkType;

(i.e. we also have the value_type be different depending on the iterator type). We should make sure this is consistent. I technically don't need this feature, it just makes for a nice example in the tests for #923.

[m-fila]

Just to confirm, the main thing that now is possible that wasn't before is this:

ExampleClusterCollection coll{};
for (auto& elem : coll) {
  coll.setEnergy(42);  // not possible before because elem would not have a setter
}

Does it compile now? I thought it still shouldn't since operator* for iterator returns temporary mutable dataobject which can't be bound to non-const l-value reference. (unlike for something like std::vector::iterator::operator* which returns real l-value reference)
Using value for (auto elem : coll) or forwarding reference for (auto&& elem : coll) works now and before

What is broken regardless of these changes is assigning a new handle to the element, I think. Or does that also change with this change? (Do we have a test for that that I simply missed?).

This is a separate problem that isn't affected by this PR. Dataobject assignment changes which internal object is held instead of changing members of internal object. For temporaries that was very confusing and made no sense so we disabled it in #743.

(i.e. we also have the value_type be different depending on the iterator type). We should make sure this is consistent. I technically don't need this feature, it just makes for a nice example in the tests for #923.

We could also make it consistent in the other direction: have both immutable and mutable iterators to link collection use immutable type as value_type. I guess the choice is whether meta-programming becomes easier (in #923) or we want to follow some of the conventions from "Container" even though we can't fulfill all of them (and it seems that actual we have so far no code that really depends whether we follow them or not)

[tmadlener]

Just to confirm, the main thing that now is possible that wasn't before is this:

ExampleClusterCollection coll{};
for (auto& elem : coll) {
coll.setEnergy(42);  // not possible before because elem would not have a setter
}

Does it compile now? I thought it still shouldn't since operator* for iterator returns temporary mutable dataobject which can't be bound to non-const l-value reference.

You are correct, it doesn't compile at the moment. But since for (auto elem : coll) works, I think I should be able to work around this in #923 without changing this.

We could also make it consistent in the other direction: have both immutable and mutable iterators to link collection use immutable type as value_type.

Yes, exactly. Looking at the possible can of worms this opens here, I think it would be easier to do that.

[m-fila]

How about we make it requirement in CollectionType concept so all the things claiming to be "collection" check it?

requires std::same_as<typename T::value_type, typename std::iterator_traits<typename T::iterator>::value_type>;
requires std::same_as<typename T::value_type, typename std::iterator_traits<typename T::const_iterator>::value_type>;