Re: [boost] [poly_collection] Andrzej's review

Subject: Re: [boost] [poly_collection] Andrzej's review
From: Joaquin M López Muñoz (joaquinlopezmunoz_at_[hidden])
Date: 2017-05-12 06:13:46

El 12/05/2017 a las 1:07, Andrzej Krzemienski via Boost escribió:
> Hi Everyone,
>
> Some comments from me about PolyCollection library, First, Joaquín, thank
> you for writing and sharing this library.

Thank you for your review!

> [...]
>
> [1]
>
> Now, because I have no practical experience with the problem, what I say in
> this paragraph might be incorrect. But I have a problem here. You will use
> this library for improved performance, maybe for a game implementation, yet
> because a game needs super-performance, can a programmer afford to use
> dynamic dispatch, or RTTI or exceptions? I always imagined taht such
> library, rather than resembling `ptr_vector<I>` would resemble
> `vector<variant<A, B, C>>`, that is: I decide and fix the container on a
> set of types I will be storing inside. This set of tyes is embedded in the
> type, so I can detect many mis-usages at compile time. The implementation
> can be faster, because the number of segments is fixed (no segment
> management), and no registration checks need to be performed. Then, iterate
> over such collection with a static_visitor (from Boost.Variant). Of course,
> this would be constraining: I need to know all the types I will be using
> ahead of time. But maybe in practice this is often the case?

I agree with you vector<variant<...>> should be faster than 
ptr_vector<I>, but this
does not invalidate the lib rationale that replacing ptr_vector<I> with 
base_collection<I>
will get you a speedup; of course the programmer has to decide on the 
various alternatives
at their disposal, with different pros and cons. I don't know either if 
vector<variant<...>>
is the "obvious" improvement over ptr_vector>I> to the extent that it 
deserves
discussing/mentioning explicitly.

On a related note, some have asked for a variant_collection to be part of
Boost.PolyCollection roadmap.

> [...]
>
> [2]
>
> I am not comfortable with per-segment functions having the same name as
> container-wide functions, and being only overloaded based on function or
> template parameters, like `cc.size()` and `cc.size(index)`. These two
> functions do different things, so they deserve different names. Maybe
> `cc.segment_size(index)`, or `cc.serment(index).size()`?

On the contrary, I like name overloading better, because it looks terser 
yet sufficiently
expressive:

   cc.size<warrior>(); // what else but the numer of warriors in the 
collection?

Furthermore, there are member functions such as

   cc.erase(begin<warrior>());

that, according to your suggestion, should be renamed as (IMHO very ugly)

   cc.segment_erase(begin<warrior>());

and if we decide that this latter example should be exempt from the segment_
prefix, then what's the rationale for what gets segment_ and what does not?

> [3]
>
> I downloaded it and tried toy examples with GCC 6.3 on Windows with
> -std=c++11, and clang 3.8.1 on Fedora with -std=c++11. It compiIes fine. I
> observed that the following small program crashes (assertion fails):
>
> ```
> #include <boost/poly_collection/base_collection.hpp>
>
> struct Iface
> {
>      Iface() = default;
>      virtual ~Iface() = 0;
> };
>
> inline Iface::~Iface() {}
>
> struct Type1 : Iface
> {
>      Type1() = default;
>      Type1(Type1&&) = delete;
> };
>
> struct Type2 : Iface
> {
>      Type2() = default;
>      Type2(Type2&&) {} // throwing move
>      // no move assignment
> };
>
> int main ()
> try {
>      boost::base_collection<Iface> c;
>      c.insert(Type1{}); // fires an assert
>      c.insert(Type2{}); // fires an assert
> }
> catch (std::exception const& e)
> {
> }
> ```
>
> Admittedly, `Type1` and `Type2` are not "acceptable", but according to the
> documentation this should throw an exception upon insertion rather than
> firing assertions.

Umm... Yes, you're right, I think the assertion in

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/poly_collection.hpp#L945

should be removed. Let me study it carefully. Thanks for spotting this.

Joaquín M López Muñoz