fix: use type std::size_t for index in Collection element accessors and size()

[c-dilks]

BEGINRELEASENOTES

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

ENDRELEASENOTES

fix #406

[tmadlener]

Thanks for working on this. I think you also have to change the definitions to match here:

podio/python/templates/Collection.cc.jinja2

Lines 31 to 45 in f4c9219

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

[tmadlener]

Thanks for spotting and fixing this.

[c-dilks]

Looks like the key4hep CI failure is also happening on master.

I can try to make VectorMembers and OneToManyRelations use size_t here too, but I may need help making sure everything that needs to change is changed.

[tmadlener]

Looks like the key4hep CI failure is also happening on master.

Yes that is a slightly annoying but known issue, see #403.

[c-dilks]

All right, I think that's all of them. I didn't bother to change the internal for loops, e.g.,

podio/python/templates/macros/implementations.jinja2

Line 198 in f4c9219

for (unsigned i = 0; i < value.{{ relation.name }}_size(); ++i) {

since that may be a bit overkill; my main concern was from the user-facing side of the generated C++ code.

I didn't touch any of the SIOBlock code, but at first glance I didn't find any user-facing unsigned types to worry about.

Do we need to add size_t anywhere in this file?

podio/python/podio/generator_utils.py

Lines 41 to 54 in f4c9219

	# Types considered to be builtin
	BUILTIN_TYPES = ["int", "long", "float", "double",
	"unsigned int", "unsigned", "unsigned long",
	"char", "short", "bool", "long long",
	"unsigned long long"]
	# Fixed width types defined in <cstdint>. Omitting int8_t and uint8_t since they
	# are often only aliases for signed char and unsigned char, which tends to break
	# expectations towards the behavior of integer types. Also omitting the _fastN_t
	# and leastN_t since those are probably already covered by the usual integer
	# types.
	ALLOWED_FIXED_WIDTH_TYPES = ["int16_t", "int32_t", "int64_t",
	"uint16_t", "uint32_t", "uint64_t"]

[tmadlener]

The BUILTIN_TYPES effectively just determine which types can be used in the yaml files to define a datamodel, so I think we don't need size_t in there. At least until someone actually requests it.

Thanks again for going through all the bits and pieces here. Waiting for CI to succeed and then I think this can be merged.

[jmcarcell]

Maybe std::size_t instead of size_t? I don't have an opinion on that but std::size_t is more C++-like

[tmadlener]

Good point. I don't have a strong opinion on it either. Strictly speaking #include <cstddef> and std::size_t would probably be the most standard conformant way to do this. For all practical purposes it probably doesn't matter too much, but since we are already here we might as well go ahead and use std::size_t and add the necessary include to the Collection.h.jinja2 template.

[c-dilks]

Good point. I don't have a strong opinion on it either. Strictly speaking #include <cstddef> and std::size_t would probably be the most standard conformant way to do this. For all practical purposes it probably doesn't matter too much, but since we are already here we might as well go ahead and use std::size_t and add the necessary include to the Collection.h.jinja2 template.

Done in f9880d6.

Do we need #include<cstddef> in Object.h.jinja2 and MutableObject.h.jinja2 too, or anywhere else?

[tmadlener]

Do we need #include<cstddef> in Object.h.jinja2 and MutableObject.h.jinja2 too, or anywhere else?

Good catch. Those should be the only ones for this PR.