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

2017-05-12 8:13 GMT+02:00 Joaquin M López Muñoz via Boost < boost@lists.boost.org>:

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.

Just to make my remark clear, I am not saying one should use vector> instead of ptr_vector<> for performance. Based on my experience OO-inheritance solutions are faster that using `variant`. As you say below, I was suggesting the alternative design: `variant_collection`.

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>());

or: cc.segment<warrior>().clear(); Assuming that I understand what the former notation does.

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?

I can see the problem you are describing. Also I agree with you that in the examples above, using the overload is not ambiguous to the users. On the other hand, I was encountering problems when trying to pass member function names as parameters to to other functions. When function `size()` is overloaded, I cannot easily pass its address to another function: some_for_each(&T:: size); // ambiguous: which overload But maybe it is just a bikeshed discussion. The library is good with either choice of names. Regards, &rzej;