Re: [boost] [review] Review of PolyCollection starts today (May 3rd)

Subject: Re: [boost] [review] Review of PolyCollection starts today (May 3rd)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2017-05-18 00:59:10

AMDG

I vote to ACCEPT PolyCollection into Boost.

Details:

General:

Missing .gitattributes

index.html:

No comments.

an_efficient_polymorphic_data_st.html:

"interspersedly"
This word sounds a bit awkward to me.

"the information is typically available at compile
time in the point of insertion in the vector"
I would say "at the point" instead of "in the point"

tutorial.html:

"Imagine we are developing a role game in C++"
Do you mean "role playing game"?

For choosing which type to insert, std::discrete_distribution
is slightly more appropriate than std::uniform_real_distribution.

"...by the user code --we will see more about this later"
This should be an em-dash.

"Insertion mechanisms are rather..."
-> "The insertion mechanisms are rather..."

"to explicitly forbid"
split infinitive

"...it is possible to only address the elements of a given
segment ..."
Address here is a bit confusing as "address" has a specific
meaning in programming.  Access is probably a better term
to use.

reference.html:

"...in the same order, and viceversa."
s/viceversa/vice versa/

"cc.empty(index)...
Throws: If the indicated type is not registered. "
Does this really make sense?  I think it would be
more useful to treat non-registered segments as
empty.  Throwing here is somewhat inconsistent
with operator== which considers non-registered
segments to be equivalent to empty segments.

"cc.max_size()
REVIEW THIS DESIGN DECISION"
"the minimum return value of max_size for each of the segments of the
collection"
I think that the fundamental invariant of
max_size should be that size() <= max_size().
Both max_size and capacity are a bit strange.
My inclination would be to remove capacity
entirely (or rename it) and change max_size to
mean the maximum possible size of the whole container.

Class template base_collection
"subobject(x) = static_cast<Derived&>(x) with typeid(x)==typeid(Derived)"
static_cast excludes virtual inhertance, which I presume is unsupported.

algorithm.hpp:

61:
segment_split: ADL (Contrived test case in test_adl1.cpp)

any_collection.hpp:

"See http://www.boost.org/libs/any_collection for library home page."
s/any_collection/poly_collection/

operator!= should be defined in the same way as operator==.
Overload resolution is subtly different for a
non-template friend function defined inline vs.
a regular free function template.  This is
especially problematic because any_collection_fwd.hpp
declares a different operator== which is not implemented.

any_collection_fwd.hpp:

http://www.boost.org/libs/any_collection again.

base_collection/function_collection have identical issues.

exception.hpp:

No comments.

detail/any_iterator.hpp:

line 74:  explicit operator Concrete*()const noexcept
This really should have an assert that the type is correct.

detail/any_model.hpp:

line 53:
!type_erasure::is_subconcept<type_erasure::assignable<>,Concept2>::value
Boost.TypeErasure was recently extended to support move
assignment (assignable<_self, _self&&>).  Should this
also be handled here?  For the record, I'm planning
to fix the constructors and assignment operators of
any so that is_constructible and is_assignable, etc.
work, but it's quite annoying to implement.

line 66, 94, 112, 123:
    type_erasure::is_subconcept<type_erasure::typeid_<>,Concept>::value
This isn't quite correct.  You need to check
typeid_<remove_cv_t<remove_reference_t<T>>>
Also it looks like this code is trying to allow an any
that doesn't use typeid_ to be stored in a segment
of anys (at least that's the impression that I get from
the definition of subobject(x) for any_collection in
reference.html).  Trying to do this will error out in
split_segment.hpp:303 in build_index when constructing
the value_type.

auto_iterator.hpp:

No comments.

base_model.hpp:

line 45: static void* subaddress(T& x){return boost::addressof(x);}
This assumes that the address of the base is the address
of the whole object which is wrong and can fail in the
case of multiple inheritance.  You need to use dynamic_cast<void*>.
(Failing test case included: test_multiple_inheritance.cpp)

callable_wrapper.hpp:

I'm somewhat curious about the rationale for
using callable_wrapper instead of std::function w/ std::ref.
Ah, I see:
- data() is needed by function_model::subaddress
- std::function can waste a lot of memory, depending on the implementation.

line 83:        return r(std::forward<Args>(args)...);
This fails to handle R = void correctly.  The return
value of the function should be ignored.
(Failing test case included: test_void_return.cpp)

callable_wrapper_iterator.hpp:

function_model.hpp:

No comments.

functional.hpp:

No comments.

integer_sequence.hpp:

This can't possibly be the best implementation.
make_index_sequence<N> and make_index_sequence<M>
with N != M generate N+M totally independent
instantiations of make_int_seq_impl.

>From the source:
"...but make_index_sequence is not hard to
implement *(if efficiency is not a concern)*"
[emphasis added]

is_acceptible.hpp, is_callable.hpp, is_constructible.hpp,
is_final.hpp, is_nothrow_eq_comparable.hpp, iterator_impl.hpp,
iterator_traits.hpp, packed_segment.hpp:

No comments.

poly_collection.hpp:

If poly_collection is defined in namespace detail,
that means that your internal functions can be
found by ADL for user defined types that mention
poly_collection.  (I always avoid putting types
that are part of the public interface in namespace
detail to avoid this problem.)  Test case in test_adl2.cpp

line 1109: "// TODO: can we do better than two passes?"
This comment seems obsolete.

segment.hpp, segment_backend.hpp, split_segment.hpp,
stride_iterator.hpp, type_restitution.hpp, value_holder.hpp

No comments.

In Christ,
Steven Watanabe