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

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>`, 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> 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> 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

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/po... should be removed. Let me study it carefully. Thanks for spotting this. Joaquín M López Muñoz