AIDASoft / podio

PODIO
GNU General Public License v3.0
23 stars 57 forks source link

Document 'Collection' comparison with 'Container' named requirement #598

Closed m-fila closed 2 months ago

m-fila commented 3 months ago

BEGINRELEASENOTES

ENDRELEASENOTES

Fixes #479, Fixes #546

The goal of this PR is to document current behavior not to fix things. It'll be easier to fix later once we known what is missing. A few methods and aliases were added but they don't collide with anything else

C++ specifies Container as a named requirement, that means specifies required types, functions, semantics and behaviors as well post requirements on its iterators.

The requirements were collected in a form of a markdown file. Next to each requirement it's indicated whether it's fulfilled by the collection/iterators. A short comment indicates any peculiar behaviors or why it isn't fulfilled. The document is accompanied by a set of tests. The tests are meant to detect changes in collection semantic and interface, and fail indicating need to update the documentation

Beside Container a short paragraph and a few tests are added for checking AllocationAwareContainer (collection uses default allocator implicitly #424), iterator adapters. Another short paragraph is added on interactions with standard algorithms (algorithms require at least LegacyInputIterator which is not fulfilled by collection iterators, the code may compile but it's std-implementation depended #150) and ranges (which use at least std::input_iterator constrain that again isn't fulfilled by collection iterators #273)

m-fila commented 2 months ago

AFAICT this is ready for review, right? One general comment this has to be included in the doc/index.rst file to show up in the documentation in the end.

Yes, please :slightly_smiling_face:

I realized that we have a detection idiom implemented in utils. I could rewrite the traits in test with it to reduce the boiler plate a bit if you think it's worth it

tmadlener commented 2 months ago

No strong opinion on the usage of the detection idiom, I leave it up to you.

m-fila commented 2 months ago

Thanks again for putting this together. I like this quite a lot, and I have mainly smaller comments (see below).

One thing that I am not sure how to handle best yet is the fact, that you stayed entirely within the STL language of using iterator and const_iterator. This makes this document self contained and it is straight forward to compare to the STL and the named requirements. However, there is no mention yet of the Iterator and MutableIterator that we generate. Do you think there is an easy way to at least mention them somewhere. I think that could reduce the potential for confusion if people look e.g. at the doxygen API documentation for EDM4hep.

Thank you very much :blush: I added in the collection table that iterator is defined as MutableCollectionIterator and similar for const_iterator. And a short paragraph at the start of iterator part that this convention is used for iterator and const_iterator

As for the detection idiom I think I'll leave it as is

tmadlener commented 2 months ago

I heard no complaints, so I will merge this.

hegner commented 2 months ago

Thanks!