|
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
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
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 acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk