Boost logo

Boost :

Subject: Re: [boost] [review] Review of PolyCollection starts today (May 3rd)
From: Steven Watanabe (watanabesj_at_[hidden])
Date: 2017-05-23 06:13:56


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

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.

>> " 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()
>> "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:
> 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
> 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;
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;
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

Boost list run by bdawes at, gregod at, cpdaniel at, john at