Formalize common interface for collections with a concept

[m-fila]

BEGINRELEASENOTES

• Formalize expected collection interface with a concept
• Add at and create to UserDataCollection

ENDRELEASENOTES

This is an idea to formalize to some degree the expected interface of a podio collection.
Currently we have three types of collections: UserDataCollection, link collection and datatype collection. Each of them implements some common methods and type aliases that aren't and can't be part of abstract class CollectionBase.
Yet from time to time we discover that one of them is missing something that the others have or worse we discover that some generic code doesn't work with some collection (for example #731, #718, #598)

The concept can be at first used in tests to ensure at compile time that none of the collections is missing part of expected interface.
Secondly it could be possibly used in derived packages to constrain templates better than std::is_base_of<podio::CollectionBase, T>

I propose to include the following:

• being a concrete class derived from CollectionBase (which also handles that all the interfaces from CollectionBase are there)
• constructors, assignments,
• implementing ranges
• constexpr static strings with human readable types (typeName), after Add typeName member to link collections #748
• typedefs,
• create(), push_back(obj),
• all combinations of begin() and end(),
• element access with operator[] and at

Perhaps something else worth having in all collections is still missing?
Datatype collections have LCIO-style operator-> but I'm not sure if the others also should have it?

[tmadlener]

Datatype collections have LCIO-style operator-> but I'm not sure if the others also should have it?

It's possible that this is mainly there for historical reasons, as at some point the goal was to make a drop-in replacement LCIO generated via podio, I think. Since LCIO is all pointers, this was introduced to mimic things. Maybe we should think about deprecating and removing it. I am not even sure it is used anywhere.

[jmcarcell]

I think for this one and the ones below the requirement of the typename to exist doesn't need to be there before since it won't compile when it reaches this requires. I guess it's there for explicitness.

[m-fila]

You are right, it still won't compile, but I it will change slightly the error message. With typename there will be an information that required type is invalid rather than going directly to the nested requirements:

/home/mafila/podio/include/podio/utilities/TypeHelpers.h: In substitution of 'template<class CollT>  requires  CollectionType<CollT> const CollT& podio::Frame::get(const std::string&) const [with CollT = podio::UserDataCollection<long unsigned int>]':
/home/mafila/podio/tests/read_test.h:39:65:   required from here
   39 |   auto& usrInts = event.get<podio::UserDataCollection<uint64_t>>("userInts");
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
/home/mafila/podio/include/podio/utilities/TypeHelpers.h:250:9:   required for the satisfaction of 'CollectionType<CollT>' [with CollT = podio::UserDataCollection<long unsigned int, void>]
/home/mafila/podio/include/podio/utilities/TypeHelpers.h:250:26:   in requirements with 'T t', 'const T ct' [with T = podio::UserDataCollection<long unsigned int, void>]
/home/mafila/podio/include/podio/utilities/TypeHelpers.h:268:12: note: the required type 'typename T::mutable_type' is invalid
  268 |   typename T::mutable_type;
      |   ~~~~~~~~~^~~~~~~~~~~~~~~~
/home/mafila/podio/include/podio/utilities/TypeHelpers.h:269:12: note: nested requirement 'convertible_to<typename T::mutable_type, typename T::value_type>' is not satisfied
  269 |   requires std::convertible_to<typename T::mutable_type, typename T::value_type>;

vs

/home/mafila/podio/include/podio/utilities/TypeHelpers.h: In substitution of 'template<class CollT>  requires  CollectionType<CollT> const CollT& podio::Frame::get(const std::string&) const [with CollT = podio::UserDataCollection<long unsigned int>]':
/home/mafila/podio/tests/read_test.h:39:65:   required from here
   39 |   auto& usrInts = event.get<podio::UserDataCollection<uint64_t>>("userInts");
      |                   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~
/home/mafila/podio/include/podio/utilities/TypeHelpers.h:250:9:   required for the satisfaction of 'CollectionType<CollT>' [with CollT = podio::UserDataCollection<long unsigned int, void>]
/home/mafila/podio/include/podio/utilities/TypeHelpers.h:250:26:   in requirements with 'T t', 'const T ct' [with T = podio::UserDataCollection<long unsigned int, void>]
/home/mafila/podio/include/podio/utilities/TypeHelpers.h:269:12: note: nested requirement 'convertible_to<typename T::mutable_type, typename T::value_type>' is not satisfied
  269 |   requires std::convertible_to<typename T::mutable_type, typename T::value_type>;

[tmadlener]

I suppose in most of the cases this shouldn't really matter as the majority of the collections are generated with the right interface in any case. I do like the more explicit message a bit better though, even if the information content / readability is probably the same at the point where you actually have to look at these errors ;)

[jmcarcell]

At least this one is already implied by std::ranges::random_access_range. Are none of the others already implied? If so they could be removed.

[m-fila]

Yes, std::random_access_iterator<typename T::iterator> is redundant here. The others checks for iterators are not. Check for non-const begin and end are redundant.
I wanted to keep them for completeness, I thought otherwise it would look like some checks are missing (For example, why there is check for const begin but no check for non-const? Ahhh it's part of range concept which is part of random_access_range concept that we check, all good)

[tmadlener]

Anything speaking against merging this as is?

[m-fila]

From my side nothing, unless we want to be less explicit and remove some redundancy as Juan suggested

[tmadlener]

I am fine with keeping some redundancy for fewer questions / riddles when reading the concept. If it becomes an issue with compile times we can always prune it down to a minimal set, IMO.