[boost] [poly_collection] Andrzej's review

Subject: [boost] [poly_collection] Andrzej's review
From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2017-05-11 23:07:39

Hi Everyone,

Some comments from me about PolyCollection library, First, Joaquín, thank
you for writing and sharing this library.

I am not an an expert in the domain. In my programs I never needed this
optimization. But I have seen it described in a number of publications, and
I am convinced it deserves a place in Boost libraries, so that people are
not forced to reinvent it.

[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?

If you disagree, maybe it is wort discussing it in the rationale.

[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()`?

[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.

[4]

The documentation is good. As to implementation, I did not look at it in
detail.

Regards,
&rzej;