Boost logo

Boost :

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
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
is the "obvious" improvement over ptr_vector>I> to the extent that it
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

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

Furthermore, there are member functions such as


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


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

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

Joaquín M López Muñoz

Boost list run by bdawes at, gregod at, cpdaniel at, john at