type inconsistency between Collection::size() and index for const object accessors

[c-dilks]

• PODIO version: master

There is a difference between the type size_t returned by size() from a Collection and the type of the index used in the const object accessors, unsigned int:

podio/python/templates/Collection.h.jinja2

Line 77 in f4c9219

size_t size() const final;

vs.

podio/python/templates/Collection.h.jinja2

Lines 94 to 101 in f4c9219

	/// Returns the const object of given index
	{{ class.bare_type }} operator[](unsigned int index) const;
	/// Returns the object of a given index
	Mutable{{ class.bare_type }} operator[](unsigned int index);
	/// Returns the const object of given index
	{{ class.bare_type }} at(unsigned int index) const;
	/// Returns the object of given index
	Mutable{{ class.bare_type }} at(unsigned int index);

Is there a particular reason for this difference?

cf. std::vector, where we have (ignoring C++20 changes for clarity):

size_type size();
reference at( size_type pos );

in other words, std::vector::at takes the same type that std::vector::size() returns.

[tmadlener]

You are right. I don't see a reason to not be consistent here. This should be fairly straight forward to fix, I think as these are directly passed to the underlying std::deque, e.g.

podio/python/templates/Collection.cc.jinja2

Lines 39 to 41 in f4c9219

	Mutable{{ class.bare_type }} {{ collection_type }}::operator[](unsigned int index) {
	return Mutable{{ class.bare_type }}(m_storage.entries[index]);
	}

Out of of curiosity: Does something change with c++20 in this respect? I am asking since you mentioned it explicitly.

[c-dilks]

According to cppreference.com it's effectively size_type no matter the version:

Also, std::deque::operator[] and std::deque::size() use size_type:

Since vector and deque both use std::allocator by default, where size_type is std::size_t, I would assume vector::size_type is the same as deque::size_type (reference SO thread).

[c-dilks]

I'm also now wondering if we should switch to size_t for VectorMembers and OneToManyRelations, but at least for those the type is always unsigned int. For example:

podio/python/templates/macros/declarations.jinja2

Lines 123 to 134 in f4c9219

	{% macro multi_relation_handling(relations, get_syntax, with_adder=False) %}
	{% for relation in relations %}
	{% if with_adder %}
	void {{ relation.setter_name(get_syntax, is_relation=True) }}({{ relation.full_type }});
	{% endif %}
	unsigned int {{ relation.name }}_size() const;
	{{ relation.full_type }} {{ relation.getter_name(get_syntax) }}(unsigned int) const;
	std::vector<{{ relation.full_type }}>::const_iterator {{ relation.name }}_begin() const;
	std::vector<{{ relation.full_type }}>::const_iterator {{ relation.name }}_end() const;
	podio::RelationRange<{{ relation.full_type }}> {{ relation.getter_name(get_syntax) }}() const;
	{% endfor %}
	{%- endmacro %}

Ultimately it looks like Collection::size() is the only odd one out, returning size_t.

[tmadlener]

It also looks like we have a size_t in the RelationRange that is one of the returns above.

podio/include/podio/RelationRange.h

Lines 31 to 33 in f4c9219

	/// convenience overload for size
	size_t size() const {
	return m_size;

For consistency with the usual c++ containers I think it would be good to have size_t everywhere. From a purely functional point of view, it doesn't really matter probably, because the ObjectID uses an int internally at the moment, and that is essentially the limiting factor for the number of elements in a collection:

podio/include/podio/ObjectID.h

Lines 9 to 10 in f4c9219

	/// index of object in collection
	int index;