[Boost-users] [review] PolyCollection formal review results: ACCEPTED into Boost

Subject: [Boost-users] [review] PolyCollection formal review results: ACCEPTED into Boost
From: Ion Gaztañaga (igaztanaga_at_[hidden])
Date: 2017-05-22 20:44:38

Hi,

Thank you for your participation in the PolyCollection review. I'll try 
to collect all votes and summarize the agreed changes on the library.

-----
Votes
-----
I count 8 (all positive) reviews and votes from

Pete Bartlett
Andrzej Krzemienski
Hans Dembinski
Thorsten Ottosen
Vinnie Falco
Edward Diener
Brook Milligan
Steven Watanabe

--------
Comments
--------

Additionally the following boosters did some positive comments on the 
library and made some questions

Dominique Devienne
Adam Wulkiewicz
Myself

No comment was blocking for the acceptance of the library so 
Boost.PolyCollection is accepted into Boost, congratulations to Joaquín!

I recommend committing the library to the boostorg repo as soon as 
possible in order to start regressions and try to include the new 
library in Boost 1.65. Many of the proposed changes are one-liners in 
the documentation and code and hopefully will be ready for Boost 1.65. 
Deeper changes can wait until Boost 1.66.

---------------------------------------
Summary of agreed comments and changes:
---------------------------------------

------------------
Dominique Devienne (positive comment)
https://lists.boost.org/Archives/boost/2017/05/234672.php
------------------

"I suspect the new poly_collection could be used for more efficient 
parallel (and/or concurrent) processing of elements, and would be 
interested in examples and even a ::parallel_for_each() algorithm that 
leverages the collection's internal storage"

Joaquín will try to play with this idea to see how it fares performancewise.

------------------
Pete Bartlett (positive review)
https://lists.boost.org/Archives/boost/2017/05/234669.php
------------------

"It may be worth remarking in the docs what facilities are used for the 
type registries that are presumably behind the scenes. From the 
reference section I think RTTI is required. Is this a hard limit or 
could Boost.TypeIndex be used?"

Additional comment from the review manager: Consider making RTTI 
customizable. Boost.TypeInfo should be easily usable. In any case 
document the use of RTTI in the documentation.

Joaquín will consider the customizable RTTI, will document that RTTI is 
required by the library.

------------------
Andrzej Krzemienski (positive review)
https://lists.boost.org/Archives/boost/2017/05/234716.php
------------------
[1] "Based on my experience OO-inheritance solutions are faster that 
using `variant`. As you say below, I was suggesting the alternative 
design: `variant_collection`."

Joaquín: "Some have asked for a variant_collection to be part of 
Boost.PolyCollection roadmap."

[2] "I am not comfortable with per-segment functions having the same 
name as container-wide functions"

Joaquín: "On the contrary, I like name overloading better, because it 
looks terser yet sufficiently expressive"

[3] "I observed that the following small program crashes (assertion 
fails). According to the documentation this should throw an exception 
upon insertion rather than firing assertions"

Joaquín: I think the assertion will be removed.

[4] Commenting c.insert(x) / c.insert(it,x) reference: "And maybe it is 
just me, but I cannot make the sense of it"

Joaquín: There is a typo that will be fixed. Will reword the reference 
sections into a more digestible form.

------------------
Ion Gaztañaga (positive comment)
https://lists.boost.org/Archives/boost/2017/05/234729.php
https://lists.boost.org/Archives/boost/2017/05/234745.php
------------------

[1] Model-based polymorphism: "Wouldn't be a good idea to make this 
model-based polymorphism (or a refined one) public? Maybe the current 
model needs more work to support additional polymorphism models."

Joaquín: I'd be really conservative here and withhold such a big move 
until real use cases are found. The problem is once this is made public 
the design of the lib becomes effectively frozen. Will study it for the 
future if there's demand.

[2] Optimize memory-indirection caused by unique_ptr: "segment_type 
could directly hold the class derived from segment_backend as 
packed_segment/split_segment will need the same size and alignment 
requirements regardless of the ConcreteType they hold (as long as they 
use std::vector). This could improve insertion times and other 
operations. This optimization could also make easier to fix the 
following section (allocator propagation)."

Joaquín: In principle if the optimization is reasonably implemented t'd 
be welcome.

[3] Allocator propagation: The library does not correctly propagate the 
allocator to support the scoped allocator model.

Joaquín will fix this.

[4] In many operations the dynamic type of the object to be inserted or 
manipulated can be  known at compile, so the internal virtual call 
overhead can be optimized. Specially in "insert" and "emplace", but many 
other operations that work with a single segment can be optimized as well.

Joaquín will review operations and will try to "staticfy" as much as 
possible.

[5] Documentation should reflect some operations, like size<T>(), 
empty<T>() and others are not constant-time cheap operations.

Thorsten suggested: The documentation should defin k = # of segments in 
container and add in those operations "Complexity: linear in k" as the 
explanation.

Joaquín will document it.

---------------
Adam Wulkiewicz (comment, but positive)
https://lists.boost.org/Archives/boost/2017/05/234662.php
---------------

[1] Polycollection doesn't meet the requirements of C++ container as 
defined in the standard, specially section (23.2.1.3).

Joaquín: The part about 23.2.1.3 is not listed among the differences, 
will include it.

---------------
Hans Dembinski (positive review)
https://lists.boost.org/Archives/boost/2017/05/234694.php
---------------

[1] "boost::function_collection was not so clear to me. It is nice to 
keep the narrative of game development going, but like someone said 
before, I think it would help to give a simpler example"

Joaquín will improve the explanation

[2] "I felt that the benefit of using boost::function_collection was not 
so clearly presented as for boost::base_collection. The function 
pointers are all the same size, so how exactly does the optimisation enter"

Joaquín will improve the explanation.

[3] "I tried to compile the tests on a Mac with Apple-clang 8.0.0. It 
worked after I manually specified "cxxflags=-std=c++11"

Note from the review manager: No fix was discovered, I suggest working 
on this as many Apple users will have this problem.

---------------
Thorsten Ottosen (positive review)
https://lists.boost.org/Archives/boost/2017/05/234697.php
---------------

[1] "OO programming and copyability of classes is not something that 
everybody is going to like. (...) Would it be possible for the library 
to pick up a private or protected copy-constructor when needed, possible 
via a friend declaration? (...) being able to have your hierarchy 
non-copyable while allowing copyability within the container is 
important IMO."

Moveability is already required to store classes in the segment. Joaquín 
will study the possibility of using a copying traits class to let users 
customize this point.

[2] "I miss range versions of the various iteration patterns so I can 
use them directly with a range-based for. E.g. instead of"

Joaquín will implement it.

[3] "Inside the segments you store std::vector. I could easily imagine 
the need to store something else, (...) Therefore I would love to have a 
second defaulted argument set to std::vector"

Element contiguity is a key property and this customization requires a 
lot of work. Deferred for the future if there is demand for it.

[4] "Implementation of equality: do we need to be set like semantics 
(two-pass)? Why don't you just delegate to the segments after checking 
the size?"

Joaquín proposed a new slightly better implementation.

[5]  "tutorial: consider using override where appropriate"

Joaquín will review code to use it.

[6] "why ==,!= but not <,>,>=, <="

Joaquín followed the interface of std::unordered_set,

[Review manager's comment: Consider explaining this in the documentation]

[7] "Clarify that

boost::poly_collection::for_each
   <warrior,juggernaut,goblin,std::string,window>( //restituted types
   c.begin(),c.end(),[&](const auto& x)

loops over all elements or only the specific"

Joaquín will clarify it.

[8] "It could be good to have a performance test of erase in the 
middle/front."

Joaquín will test it.

[9] "using the identifier "concept" may require change in the near future?"s

Joaquín will change it.

[10] "I wouldn't mind seeing separate graphs for 0-300 elements."

Joaquín will add it.

[11] 2clarify test are without use of reserve"

Joaquín will clarify it.

[12] The test are testing best possible scenario; a more realistic test 
would be to copy pointers to an std::vector<base*>, shuffle it and run 
update on that.

Joaquín will consider this, maybe additionally for 1-1000 elements.

---------------
Vinnie Falco (Positive review)
https://lists.boost.org/Archives/boost/2017/05/234704.php
---------------

[1] "Iterators returned by begin<T>() produce a long, ugly compile error 
when compared against iterators returned by begin<U>() or end<U>()."

Joaquín will study the issue. He's not sure if this can be improved.

---------------
Edward Diener (positive review)
https://lists.boost.org/Archives/boost/2017/05/234762.php
---------------

[1] "It is also hard to understand what the advantage of a poly 
collection of these objects entail over a more normal C++ collection ( 
vector, array etc. ) of std::function<Signature> objects, since the 
latter object type already represents a generalized callable construct 
in C++."

Joaquín will think about how to make the intro part more accessible.

[2] "I have mentioned previously that I think the documentation should 
be more specific about the types being used for the 
boost::function_collection"

Joaquín will take these points, and try to improve docs so as to make 
these aspects clearer.

---------------
Brook Milligan (positive review)
https://lists.boost.org/Archives/boost/2017/05/234872.php
---------------

[1] "The only problem was that the following test cases needed 
-std=c++14 (rather than c++11) because std::make_unique is not defined 
for c++11 on that compiler: algorithms, basic_function, and 
segmented_structure.
Perhaps a mention of the need for c++14 in some examples?"

Joaquín will fix in the corresponding Jamfile.v2 and will mention it in 
the examples.

---------------
Steven Watanabe (positive review)
https://lists.boost.org/Archives/boost/2017/05/234877.php
---------------

[1] Several documentation fixes

Joaquín will apply recommended fixes

[2] Change 
type_erasure::is_subconcept<type_erasure::assignable<>,Concept2>::value 
as Boost.TypeErasure was recently extended to support move
assignment.

[3] Fix 
"type_erasure::is_subconcept<type_erasure::typeid_<>,Concept>::value " 
with "typeid_<remove_cv_t<remove_reference_t<T>>> "

[4] Fix "static void* subaddress(T& x){return boost::addressof(x);}" as 
it does not support multiple inheritance

[5] Fix "return r(std::forward<Args>(args)...);" that does not correctly 
handle "void" return types.

Joaquín will fix all of them

[6] "make_index_sequence<N>" can be implemented more efficiently:

Joaquín will find a better implementation.

Review manager's comment: see 
https://stackoverflow.com/questions/17424477/implementation-c14-make-integer-sequence

[7] poly_collection is defined in namespace detail, internal functions 
can be found by ADL

Joaquín will try to limit what ADL can find.