|
|
Boost : |
Subject: Re: [boost] [review] Review of PolyCollection starts today (May 3rd)
From: Joaquin M López Muñoz (joaquinlopezmunoz_at_[hidden])
Date: 2017-05-22 17:21:15
El 18/05/2017 a las 2:59, Steven Watanabe via Boost escribió:
> AMDG
>
> 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
> [...]
>
> 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."
> "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"
You're right, noted.
> tutorial.html:
>
> "Imagine we are developing a role game in C++"
> Do you mean "role playing game"?
Yep, will change that.
> For choosing which type to insert, std::discrete_distribution
> is slightly more appropriate than std::uniform_real_distribution.
Your're right, will switch to std::discrete_distribution.
> "...by the user code --we will see more about this later"
> This should be an em-dash.
Noted.
> "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.
> "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... By the way, there are other instances of split
infinitive in this
section, for instance "to only address" (mentioned below).
> "...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.
> reference.html:
>
> "...in the same order, and viceversa."
> s/viceversa/vice versa/
Noted. It is "viceversa" in Spanish, hence the mistake.
> "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.
> "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.
> 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?
(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_collection.hpp:
>
> "See http://www.boost.org/libs/any_collection for library home page."
> s/any_collection/poly_collection/
Right, will change it.
> 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.
You're absolutely right, will fix it.
> any_collection_fwd.hpp:
>
> http://www.boost.org/libs/any_collection again.
>
> base_collection/function_collection have identical issues.
Noted again.
> [...]
>
> detail/any_iterator.hpp:
>
> line 74: explicit operator Concrete*()const noexcept
> This really should have an assert that the type is correct.
I don't see the need as any_iterator is a detail class not directly
exposed to
user code.
> 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?
Right, in fact now that move assignment is provided (didn't notice that)
we should
check this rather than copy assignment.
> 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.
That'd be a boon.
> 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>>>
Noted, will change it.
> 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:
#include <boost/poly_collection/any_collection.hpp>
int main()
{
using concept=boost::mpl::vector<
boost::type_erasure::copy_constructible<>,
boost::type_erasure::relaxed
>;
boost::any_collection<concept> c;
auto a=boost::type_erasure::any<concept>{0};
c.insert(a);
}
> [...]
>
> 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)
Absolutely, will change it. See also my comment above re
"static_cast<Derived&>(x)".
> 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?
> [...]
>
> 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?
> [...]
>
> 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
> line 1109: "// TODO: can we do better than two passes?"
> This comment seems obsolete.
There's an implicit second pass in
> [...]
>
> In Christ,
> Steven Watanabe
Thank you for your very valuable review! I'm bringing back home lots of
useful suggestions.
Best regards,
Joaquín M López Muñoz
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk