Boost logo

Boost Users :

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.


Boost-users list run by williamkempf at hotmail.com, kalb at libertysoft.com, bjorn.karlsson at readsoft.com, gregod at cs.rpi.edu, wekempf at cox.net