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-23 06:13:56

AMDG

On 05/22/2017 11:21 AM, Joaquin M López Muñoz via Boost wrote:
> El 18/05/2017 a las 2:59, Steven Watanabe via Boost escribió:
>> I vote to ACCEPT PolyCollection into Boost.
> 
> Thank you Steven! Sorry it took me so long to answer your post, got a busy
> weekend and yours is not a review one can deal with in a hurry.
> 
>> Details:
>>
>> General:
>>
>> Missing .gitattributes
> 
> Noted. By the way, I couldn't find this as a requirement in
> http://www.boost.org/development/requirements.html
> 

The only reason I notice is that I can't open
any of the files in notepad...

>> [...]
>>
>> an_efficient_polymorphic_data_st.html:
>>
>> "interspersedly"
>> This word sounds a bit awkward to me.
> 
> Any less awkward synonim? For other readers' convenience, the statement in
> which the word appears is:
> 
> "This mechanism is rendered mostly useless when derived1, derived2 and
> derived3 elements are traversed interspersedly without a definite pattern."
> 

  I think the sentence should be reordered.  The
problem, for me, is the use of -ly with a past
participle, which affects most direct synonyms as well.
"...useless for interspersed traversal of derived1..."
would be one possibility.

> 
>> "Insertion mechanisms are rather..."
>> -> "The insertion mechanisms are rather..."
> 
> Not being a native English speaker (nor Russian either, who typically have
> a hard time with definite articles :-) ), "Insertion mechanisms" looks
> perfectly
> OK to me, but I can change that anyway.
> 

  Since you're talking about a specific set of
insertion mechanisms, I think it should have
an article.

>> "to explicitly forbid"
>> split infinitive
> 
> At the risk of starting a style war, it is my understanding that split
> infinitives are
> okay when used wth caution. Here, the alternative:
> 
> "to forbid explicitly copying at"
> 
> seems (to me) less clear as it might be parsed as referring to some sort of
> explicit copying...

  It's not ambiguous.  "Explicitly" modifies "copying."
I.e., this isn't a correct rewrite of the sentence.
It would be either "explicitly to forbid copying"
(which is correct and unambiguous, but sounds pedantic)
or "to forbid copying explicitly"

> By the way, there are other instances of split
> infinitive in this
> section, for instance "to only address" (mentioned below).
> 

  In this case "to address only" works fine.  Although
it changes "only" to modify "the elements" instead of
"to address", this doesn't affect the overall meaning.

>> "...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.
> 
> Will change that.
> 
>> "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.
> 
> Non-registered == empty would lead to stranger situations, IMHO: we
> can't hold the invariant that empty(index)==(begin(index)==end(index)) as
> begin/end(index) cannot return anything sensible for non-registered
> segments. Similarly, max_size, capacity etc. need to forward to the
> underlying segment to return something, unless we make up the return
> values for non-registered segments.
> 

  The iterators are not dereferenceable, so they
don't actually need to point to anything.  capacity
is obviously 0.  max_size... yeah, I can see
that that would be a problem.

>> "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.
> 
> This is a design area I'm definitely not happy with. Please follow this
> thread
> for a related discussion:
> 
> https://lists.boost.org/Archives/boost/2016/11/231784.php
> 
> I'm leaning towards dropping (global) max_size/capacity, as you and
> Thorsten
> suggest, till we find sensible semantics for them.
> 

I don't think that there are sensible
semantics for capacity.

Expected semantics of capacity:
a) size() <= capacity()
b) if N <= (capacity() - size()) then N elements can
   be inserted without reallocation.
c) capacity() - size() is proportional to the amount
   of wasted memory.
d) reserve(N) has a post-condition that capacity() >= N
In particular, I think there's no way to satisfy
both (b) and (c) simultaneously.

On a related note, reserve(size() + N) probably has
unexpected behavior.

>> 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.
> 
> Ummm.. Hadn't thought about virtual inheritance. At first blush, seems
> like virtual
> inheritance would be automatically supported if only we fix subaddress as
> you suggest below, don't you think?
> 

  I think so.  If there's anywhere else that you use
a downcast (perhaps stride_iterator.hpp:83), it
would also need dynamic_cast.  If this causes any
noticeable performance cost, I don't think it's
worthwhile.  Who uses virtual inheritance outside
of iostreams?

> (the "static_cast<Derived&>(x)" you refer to is only shown for
> explanatory purposes
> and does not appear anywhere in the actual code, as you probably guessed).
> 
>> algorithm.hpp:
>>
>> 61:
>> segment_split: ADL (Contrived test case in test_adl1.cpp)
> 
> Noted. Please enlighten me: is it OK to qualify the offending call as
> detail::segment_split, or should I fully qualify as
> ::boost::poly_collection::detail::segment_split?
> 

  Any namespace is sufficient to suppress ADL.
Putting parentheses around the function name
also works.  (Personally, I always fully qualify
everything in headers, but that gets very tedious.)

>> 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).
> 
> That's the intention. If I can't get the wrapped object I'll store the any
> objects at least.
> 
>> Trying to do this will error out in
>> split_segment.hpp:303 in build_index when constructing
>> the value_type.
> 
> I'm not seeing this. First of all, there's a bug in
> 
> https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/any_model.hpp#L128
> 
> 
> s/any_cast<void*>/any_cast<const void*>. Once you do that, the following
> compiles fine:
> 
> <snip>
>     using concept=boost::mpl::vector<
>       boost::type_erasure::copy_constructible<>,
>       boost::type_erasure::relaxed
>     >;
> <snip>
> 

This compiles because relaxed implies typeid_.

>> callable_wrapper.hpp:
>>
>> [...]
>>
>> 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)
> 
> Got it. I guess I only need to
> 
> s/return r(std::forward<Args>(args)...)/return
> static_cast<R>(r(std::forward<Args>(args)...))
> 
> right?
> 

  That makes it too permissive although
it's probably okay since you also construct
a std::function.

>> [...]
>>
>> 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]
> 
> Do you know of some good quality logarithmic implementation that I
> can rip?
> 

I typically use something like this:
template<bool IsBaseCase>
struct make_int_seq_impl;
template<>
struct make_int_seq_impl<false>
{
  template<class T, T N>
  using apply = typename next_integer_sequence<make_int_seq_impl<(N-1 ==
0)>::template apply<T, N-1>>::type;
};
template<>
struct make_int_seq_impl<true>
{
  template<class T, T N>
  using apply = integer_sequence<T>;
};

which is optimized for memoization rather than
for the cost of a single instantiation.

Actually, looking at the way you're using it,
it probably doesn't matter at all.

>> [...]
>>
>> 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
> 
> I see. What's best?
> 
> a) moving poly_collection to boost::poly_collection::poly_collection_detail
> b) moving poly_collection to
> boost::poly_collection::detail::poly_collection_detail
> 

I don't think it matters.  Either one should work.

In Christ,
Steven Watanabe