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

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(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(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/po...

s/any_cast/any_cast. 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 using apply = typename next_integer_sequence::template apply>::type; }; template<> struct make_int_seq_impl<true> { template 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