-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Document 'Collection' comparison with 'Container' named requirement #598
Conversation
…ence_type` required by container
…ases to `UserDataCollection`
…docs, add more checks
Co-authored-by: hegner <[email protected]>
Co-authored-by: tmadlener <[email protected]>
…ad of ! (as suggested in catch2 docs)
9850736
to
e25de51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂 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 |
No strong opinion on the usage of the detection idiom, I leave it up to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
fix typos fix swap docs clarify iterator as CollectionIterator fix destructor behaviour docs add standard algorithm links and warnings Co-authored-by: tmadlener <[email protected]>
Thank you very much 😊 As for the detection idiom I think I'll leave it as is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy with this as it is. In case we really want to go through all > 100 STL algorithms and document them we can do that later as well, but for now this should serve as a very good basis and we have something that we can refer to for capabilities.
@hegner do you want to have another look?
I heard no complaints, so I will merge this. |
Thanks! |
BEGINRELEASENOTES
cbegin
,cend
,max_size
and aliasessize_type
,difference_type
to collectionENDRELEASENOTES
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)