Quantcast

[review] Review of PolyCollection starts today (May 3rd)

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
50 messages Options
123
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
Hi everyone,

The formal review of Joaquín M. López Muñoz's PolyCollection library
starts today.

I'd like to encourage your participation as the proposed library is
small and focused, and reviewers don't need to be domain experts to
appreciate the potential usefulness of the library and propose improvements.

PolyCollection implements fast containers of polymorphic objects.
Typically, polymorphic objects cannot be stored directly in regular
containers and need be accessed through an indirection pointer, which
introduces performance problems related to CPU caching and branch
prediction. Boost.PolyCollection implements a novel data structure that
is able to contiguously store polymorphic objects without such
indirection, thus providing a value-semantics user interface and better
performance. Three polymorphic collections are provided:

* boost::base_collection
* boost::function_collection
* boost::any_collection

dealing respectively with classic base/derived or OOP polymorphism,
function wrapping in the spirit of std::function and so-called duck
typing as implemented by Boost.TypeErasure.

The library can be found here:

  Incubator:
   http://blincubator.com/bi_library/polycollection/?gform_post_id=1643

  Github:
   https://github.com/joaquintides/poly_collection

and the documentation here:

http://rawgit.com/joaquintides/poly_collection/website/doc/html/index.html

Please post your comments and review to the boost mailing list
(preferably), the Boost Library Incubator. or privately to the Review
Manager (to me ;-). Here are some questions you might want to answer in
your review:

- What is your evaluation of the design?

- What is your evaluation of the implementation?

- What is your evaluation of the documentation?

- What is your evaluation of the potential usefulness of the library?

- Did you try to use the library? With what compiler? Did you have any
problems?

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

- Are you knowledgeable about the problem domain?

And most importantly:

- Do you think the library should be accepted as a Boost library?

For more information about Boost Formal Review Process, see:
http://www.boost.org/community/reviews.html

Waiting your reviews!

Ion

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] [poly_collection] Reminder: Review of PolyCollection started May 3rd

Boost - Dev mailing list
I know a lot of boosters are willing to write a review of this great
library, so please don't be shy!

Best,

Ion

On 03/05/2017 1:46, Ion Gaztañaga via Boost wrote:

> Hi everyone,
>
> The formal review of Joaquín M. López Muñoz's PolyCollection library
> starts today.
>
> I'd like to encourage your participation as the proposed library is
> small and focused, and reviewers don't need to be domain experts to
> appreciate the potential usefulness of the library and propose
> improvements.
>
> PolyCollection implements fast containers of polymorphic objects.
> Typically, polymorphic objects cannot be stored directly in regular
> containers and need be accessed through an indirection pointer, which
> introduces performance problems related to CPU caching and branch
> prediction. Boost.PolyCollection implements a novel data structure that
> is able to contiguously store polymorphic objects without such
> indirection, thus providing a value-semantics user interface and better
> performance. Three polymorphic collections are provided:
>
> * boost::base_collection
> * boost::function_collection
> * boost::any_collection
>
> dealing respectively with classic base/derived or OOP polymorphism,
> function wrapping in the spirit of std::function and so-called duck
> typing as implemented by Boost.TypeErasure.
>
> The library can be found here:
>
>  Incubator:
>   http://blincubator.com/bi_library/polycollection/?gform_post_id=1643
>
>  Github:
>   https://github.com/joaquintides/poly_collection
>
> and the documentation here:
>
> http://rawgit.com/joaquintides/poly_collection/website/doc/html/index.html
>
> Please post your comments and review to the boost mailing list
> (preferably), the Boost Library Incubator. or privately to the Review
> Manager (to me ;-). Here are some questions you might want to answer in
> your review:
>
> - What is your evaluation of the design?
>
> - What is your evaluation of the implementation?
>
> - What is your evaluation of the documentation?
>
> - What is your evaluation of the potential usefulness of the library?
>
> - Did you try to use the library? With what compiler? Did you have any
> problems?
>
> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
>
> - Are you knowledgeable about the problem domain?
>
> And most importantly:
>
> - Do you think the library should be accepted as a Boost library?
>
> For more information about Boost Formal Review Process, see:
> http://www.boost.org/community/reviews.html
>
> Waiting your reviews!
>
> Ion

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 5/2/2017 7:46 PM, Ion Gaztañaga via Boost wrote:

> Hi everyone,
>
> The formal review of Joaquín M. López Muñoz's PolyCollection library
> starts today.
>
> I'd like to encourage your participation as the proposed library is
> small and focused, and reviewers don't need to be domain experts to
> appreciate the potential usefulness of the library and propose
> improvements.
>
> PolyCollection implements fast containers of polymorphic objects.
> Typically, polymorphic objects cannot be stored directly in regular
> containers and need be accessed through an indirection pointer, which
> introduces performance problems related to CPU caching and branch
> prediction. Boost.PolyCollection implements a novel data structure that
> is able to contiguously store polymorphic objects without such
> indirection, thus providing a value-semantics user interface and better
> performance. Three polymorphic collections are provided:
>
> * boost::base_collection
> * boost::function_collection

I could not understand from the documentation what it is I am supposed
to be inserting into a function_collection. The tutorial did not explain
this to me and the reference's technical explanation on this eluded me.

As usual the tutorial-reference form of documentation befuddles me
whereas a simple explanation of the main topics of a library would
probably make it easy for me to understand a library.

> * boost::any_collection
>
> dealing respectively with classic base/derived or OOP polymorphism,
> function wrapping in the spirit of std::function and so-called duck
> typing as implemented by Boost.TypeErasure.
>
> The library can be found here:
>
>   Incubator:
>    http://blincubator.com/bi_library/polycollection/?gform_post_id=1643
>
>   Github:
>    https://github.com/joaquintides/poly_collection
>
> and the documentation here:
>
> http://rawgit.com/joaquintides/poly_collection/website/doc/html/index.html
>
> Please post your comments and review to the boost mailing list
> (preferably), the Boost Library Incubator. or privately to the Review
> Manager (to me ;-). Here are some questions you might want to answer in
> your review:
>
> - What is your evaluation of the design?
>
> - What is your evaluation of the implementation?
>
> - What is your evaluation of the documentation?
>
> - What is your evaluation of the potential usefulness of the library?
>
> - Did you try to use the library? With what compiler? Did you have any
> problems?
>
> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?
>
> - Are you knowledgeable about the problem domain?
>
> And most importantly:
>
> - Do you think the library should be accepted as a Boost library?
>
> For more information about Boost Formal Review Process, see:
> http://www.boost.org/community/reviews.html
>
> Waiting your reviews!


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
El 06/05/2017 a las 5:53, Edward Diener via Boost escribió:

> On 5/2/2017 7:46 PM, Ion Gaztañaga via Boost wrote:
>> Hi everyone,
>>
>> The formal review of Joaquín M. López Muñoz's PolyCollection library
>> starts today.
>> [...]
>>
>> * boost::function_collection
>
> I could not understand from the documentation what it is I am supposed
> to be
> inserting into a function_collection. The tutorial did not explain
> this to me and the
> reference's technical explanation on this eluded me.

You can insert into a boost::function_collection<Signature> any entity
that a
std::function<Signature> object could be constructed from. For instance,
boost::function_collection<double(int)> holds callable entities that can
be invoked with
a (convertible to) int argument and return a (convertible to) double result:

   double f(int n){return (double)n;}

   struct g
   {
     double operator()(int n)const{return (double)2*n;}
   };

   boost::function_collection<double(int)> c;
   c.insert(&f);
   c.insert(g{});
   double factor=3;
   c.insert([=](int n){return factor*n;});

   for(const auto& x:c)std::cout<<x(1)<<" "; // prints 3 1 2

This is explained in http://tinyurl.com/kddv9nz . Didn't you find this
section
sufficiently explanatory?

Joaquín M López Muñoz

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] [poly_collection] Reminder: Review of PolyCollection started May 3rd

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Le 05/05/2017 à 22:10, Ion Gaztañaga via Boost a écrit :
> I know a lot of boosters are willing to write a review of this great
> library, so please don't be shy!
I've started to rad the documentation. The library is so good (as usual
from JMLM) that I've not yet added a single issue to my possible review.
This kind of boring ;-)

Hope I will have/take the time to send a review soon.

Best,
Vicente

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 5/6/2017 5:07 AM, Joaquin M López Muñoz via Boost wrote:

> El 06/05/2017 a las 5:53, Edward Diener via Boost escribió:
>> On 5/2/2017 7:46 PM, Ion Gaztañaga via Boost wrote:
>>> Hi everyone,
>>>
>>> The formal review of Joaquín M. López Muñoz's PolyCollection library
>>> starts today.
>>> [...]
>>>
>>> * boost::function_collection
>>
>> I could not understand from the documentation what it is I am supposed
>> to be
>> inserting into a function_collection. The tutorial did not explain
>> this to me and the
>> reference's technical explanation on this eluded me.
>
> You can insert into a boost::function_collection<Signature> any entity
> that a
> std::function<Signature> object could be constructed from. For instance,
> boost::function_collection<double(int)> holds callable entities that can
> be invoked with
> a (convertible to) int argument and return a (convertible to) double
> result:
>
>    double f(int n){return (double)n;}
>
>    struct g
>    {
>      double operator()(int n)const{return (double)2*n;}
>    };
>
>    boost::function_collection<double(int)> c;
>    c.insert(&f);
>    c.insert(g{});
>    double factor=3;
>    c.insert([=](int n){return factor*n;});
>
>    for(const auto& x:c)std::cout<<x(1)<<" "; // prints 3 1 2
>
> This is explained in http://tinyurl.com/kddv9nz . Didn't you find this
> section
> sufficiently explanatory?

The explanation you cite with the link above is a tutorial, not an
explanation. Furthermore, perhaps for the sake of syntactical brevity,
you cite an example in which a lambda function returns another lambda
function !!! This may be exceedingly clever but it can hardly be a
mainstream explanation of your functionality which most programmers
would easily understand as how a function collection "works". Your
examples above are a much better and more mainstream example of how the
function_collection works, and even the short explanation you have given
me should have been in the documentation.

It is also hard to understand what the advantage of a poly collection of
these objects entail over a more normal C++ collection ( vector, array
etc. ) of std::function<Signature> objects, since the latter object type
already represents a generalized callable construct in C++. Some of your
performance tests suggest there is little or no advantage, so a
discussion of this would have been welcome in the documentation.

>
> Joaquín M López Muñoz


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
El 06/05/2017 a las 16:49, Edward Diener via Boost escribió:

> The explanation you cite with the link above is a tutorial, not an
> explanation. Furthermore, perhaps for the sake of syntactical brevity,
> you cite an example in which a lambda function returns another lambda
> function !!! This may be exceedingly clever but it can hardly be a
> mainstream explanation of your functionality which most programmers
> would easily understand as how a function collection "works". Your
> examples above are a much better and more mainstream example of how
> the function_collection works, and even the short explanation you have
> given me should have been in the documentation.
>

Will think about how to make the intro part more accessible along the
lines you suggest.

> It is also hard to understand what the advantage of a poly collection
> of these objects entail over a more normal C++ collection ( vector,
> array etc. ) of std::function<Signature> objects, since the latter
> object type already represents a generalized callable construct in
> C++. Some of your performance tests suggest there is little or no
> advantage, so a discussion of this would have been welcome in the
> documentation.

Performance is actually the *only* reason for using Boost.PolyCollection
as opposed to say
std:.vector<std::function<Signature>>. Perfomance tests are not very
conclusive about insertion
times (VS favors Boost.PolyCollection, GCC and Clang
std:.vector<std::function<>>), but processing
times, which are the crux of the matter, definitely show the point of
the library. Admittedly this
section should be added a comments section where the results are
discussed --otherwise they
might be hard to interpret.

Joaquín M López Muñoz

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Ion Gaztañaga Via Boost wrote:

> The formal review of Joaquín M. López Muñoz's PolyCollection library
> starts today.
>
> I'd like to encourage your participation as the proposed library is
> small and focused, and reviewers don't need to be domain experts to
> appreciate the potential usefulness of the library and propose
> improvements.
>
> PolyCollection implements fast containers of polymorphic objects.
> Typically, polymorphic objects cannot be stored directly in regular
> containers and need be accessed through an indirection pointer, which
> introduces performance problems related to CPU caching and branch
> prediction. Boost.PolyCollection implements a novel data structure
> that is able to contiguously store polymorphic objects without such
> indirection, thus providing a value-semantics user interface and
> better performance. Three polymorphic collections are provided:

The library looks very well and I think it'd be a valuable contribution
to Boost.

But I'm wondering, would it make sense to expand the scope of the
library? Besides storing polymorphic data of the same type based on
run-time info to improve caching there are situations when data might be
divided into hot and cold data based on compile-time info. E.g. a
"vector" of tuples could internally store tuple of vectors and provide
interface allowing to access tuples more or less transparently. Storing
hot and cold data in different but contigeous parts of memory would also
improve caching. Since the principle is similar, would it have sense to
have both types of collections in one library? Not necessarily now, but
if this was a good idea then the name of the library would have to be
changed into something more general.

I noticed that in the documentation a word "container" is used WRT poly
collections. AFAIU this is not correct because poly_collection doesn't
meet the requirements of C++ container as defined in the standard, i.e.
allocator_traits<allocator_type>::construct should be called only for
the element type (23.2.1.3) but poly_collection contains
std::unordered_map and packed_segment contains std::vector which
constructs objects of their own value types. Also Models call operator
new in make function. If that's correct and I'm not missing anything
then I suggest using the word "collection" consistently in the docs.

Regards,
Adam

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
El 08/05/2017 a las 15:48, Adam Wulkiewicz via Boost escribió:
> Ion Gaztañaga Via Boost wrote:
>> The formal review of Joaquín M. López Muñoz's PolyCollection library
>> starts today. [...]
> The library looks very well and I think it'd be a valuable
> contribution to Boost.

Thank you!

> But I'm wondering, would it make sense to expand the scope of the
> library? Besides storing polymorphic data of the same type based on
> run-time info to improve caching there are situations when data might
> be divided into hot and cold data based on compile-time info. E.g. a
> "vector" of tuples could internally store tuple of vectors and provide
> interface allowing to access tuples more or less transparently.
> Storing hot and cold data in different but contigeous parts of memory
> would also improve caching. Since the principle is similar, would it
> have sense to have both types of collections in one library? Not
> necessarily now, but if this was a good idea then the name of the
> library would have to be changed into something more general.

IMHO this is an entirely different beast that happens to share the same
underlying data
structure as Boost.PolyCollection, but the apropriate interface would
look different: no
type registration, no type restitution, typed local_iterator's should
have to go, etc. I think
this would be best served by another, unrelated library.

> I noticed that in the documentation a word "container" is used WRT
> poly collections. AFAIU this is not correct because poly_collection
> doesn't meet the requirements of C++ container as defined in the
> standard, i.e. allocator_traits<allocator_type>::construct should be
> called only for the element type (23.2.1.3) but poly_collection
> contains std::unordered_map and packed_segment contains std::vector
> which constructs objects of their own value types. Also Models call
> operator new in make function. If that's correct and I'm not missing
> anything then I suggest using the word "collection" consistently in
> the docs.

Throughout the docs the word "container" is used in a non-standardese
sense, as you
correctly point out. On the reference, however, the concept
PolymorphicCollection is
defined as a refinement of PolymorphicContainer
(http://tinyurl.com/m9yh3hq ), which
is explicitly defined as resembling, but not the same as a standard
Container. The part
about 23.2.1.3 is not listed among the differences, and I should include
it as well (thanks!).
BTW, the standard itself is rather lousy with respect to the Container
concept. For instance,
[forwardlist.overview]/1 says

   "A forward_list is a container that supports forward iterators..."

only to state in the next parapragh that

   "A forward_list satisfies all of the requirements of a container
(Table 96), except that
   the size() member function is not provided and operator== has linear
complexity."

So, in the end, forward_list is *not* a container :-)

Joaquín M López Muñoz




_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 9/05/2017 01:48, Adam Wulkiewicz wrote:
> I noticed that in the documentation a word "container" is used WRT poly
> collections. AFAIU this is not correct because poly_collection doesn't
> meet the requirements of C++ container as defined in the standard, i.e.
> allocator_traits<allocator_type>::construct should be called only for
> the element type (23.2.1.3)

On that note, do you know of a more-comprehensible-than-standardese
guide on how allocator_traits is supposed to be used for node-based
custom container types?  I've tried to figure it out a couple times but
I'm sure I'm missing important points.



_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Hi,

I am not a Boost author, but I think I am able to give a review of PolyCollection, so here it goes.

> On 03 May 2017, at 01:46, Ion Gaztañaga via Boost <[hidden email]> wrote:
>
> Please post your comments and review to the boost mailing list (preferably), the Boost Library Incubator. or privately to the Review Manager (to me ;-). Here are some questions you might want to answer in your review:
>
> - What is your evaluation of the design?

I only looked at the interface and the description of how it works, both is great. There is nothing I would do differently.

Concerning the interface, I expected the collections to implement the std::set interface, which they mostly do. Of course, in addition they have the extra interface to do type-specific stuff. The std::set methods count, find, equal_range, lower_bound, upper_bound are missing, but the library has equivalent free functions, which I like better anyway.

> - What is your evaluation of the implementation?

I didn't look at the code. The concept as described in the documentation made a lot of sense.

> - What is your evaluation of the documentation?

I really liked the graphics which explain how objects are stored internally. The quality of the documentation is very high. Everything I looked at was very professional and made sense.

Some specifics on the tutorial section:
I understood boost::base_collection right away. This part is perfect from my POV.

boost::function_collection was not so clear to me. It is nice to keep the narrative of game development going, but like someone said before, I think it would help to give a simpler example. I felt that the benefit of using boost::function_collection was not so clearly presented as for boost::base_collection. The function pointers are all the same size, so how exactly does the optimisation enter? I assume it is not about memory allocation here, but about sorting equal function signatures into homogeneous blocks to improve branch prediction and locality, but I am not sure.

The description of boost::any_collection was clear again.

> - What is your evaluation of the potential usefulness of the library?

The library optimises the use of collections of polymorphic objects. I can see many applications. The optimisation feels so obvious now after reading the documentation that I wonder why such a library was not added sooner. It is a good thing that Boost provides a standard implementation of this optimisation so that developers don't have to reinvent it in their domain.

> - Did you try to use the library? With what compiler? Did you have any problems?

I tried to compile the tests on a Mac with Apple-clang 8.0.0. It worked after I manually specified "cxxflags=-std=c++11" in the b2 call. By the way, it would be great if b2 used the highest standard that is available by default.

> - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

I studied the documentation, mostly the tutorial. I also studied the benchmarks with interest, and I glanced at the reference. I tried to compile the tests. All in all, I guess I spend 3 hours.

> - Are you knowledgeable about the problem domain?

Yes, as it is not very specific...

> - Do you think the library should be accepted as a Boost library?

Yes.

Best regards,
Hans

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
El 10/05/2017 a las 17:56, Hans Dembinski via Boost escribió:
> Hi,
>
> I am not a Boost author, but I think I am able to give a review of PolyCollection, so here it goes.

Thanks for your review!

> [...]
>
>> - What is your evaluation of the documentation?
> [...]
>
> boost::function_collection was not so clear to me. It is nice to keep the narrative of game development going, but like someone said before, I think it would help to give a simpler example. I felt that the benefit of using boost::function_collection was not so clearly presented as for boost::base_collection. The function pointers are all the same size, so how exactly does the optimisation enter? I assume it is not about memory allocation here, but about sorting equal function signatures into homogeneous blocks to improve branch prediction and locality, but I am not sure.

I have to think over the boost::function_collection given that it does't
look as clear as
I expected... As you correctly guess my aim was to have a single
narrative, but seems
like that's not working so well.

Regarding the benefits of boost::function_collection<Signature> with
respect to
std::vector<std::function<Signature>>, memory allocation improves as well:
except where small object optimization applies, each std::function
object stores a
pointer to a heap-allocated copy of the wrapped callable entity passed
by the
user; boost::base_collection, on the contrary, stores same-type callable
entities
contiguously.

(If you are really curious, a segment of a boost::function_collection is
composed of
*two* vectors, one for the callable entities proper and another storing
std::function-like
objects pointing to the former. Think of it as a
std::vector<std::function<Signature>>
constructed with such a smart memory allocator that (same-type) wrapped
callable
entities all happen to lie sequentially in memory.)

> [...]
>
>> - Did you try to use the library? With what compiler? Did you have any problems?
> I tried to compile the tests on a Mac with Apple-clang 8.0.0. It worked after I manually specified "cxxflags=-std=c++11" in the b2 call. By the way, it would be great if b2 used the highest standard that is available by default.

It's weird because the Jamfile does have this flag already:

https://github.com/joaquintides/poly_collection/blob/master/test/Jamfile.v2

I guess the snapshot provided from the Incubator might be oldish and not
include that... I can't check right now because blincubator.com is not
responding :-(

> [...]
>
>> - Do you think the library should be accepted as a Boost library?
> Yes.

Thank you!

Joaquín M López Muñoz

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Hi Joaquín (and Ion),

I took a look at this a few months ago, and my view is positive today as
it was then. The code looks clean, the documentation good, and the tests
look thorough.

Summary: The library provides containers that offer great data locality
and which stores static type information. The net result is that
increased performance can be had, sometimes significantly. To achieve
this, objects are stored in per-type segments. Small buffer
optimizations may be advantages for vector<any> or
vector<function<...>>, but this library goes beyond that. The price you
pay is that iterators are forward only (so no sorting) and in-the-middle
erase could be slower (in some cases). I would mostly be using
base_collection in my work, but I can definitely see the need for the
other two collections. To get the optimization offered by this library
you would have to do something like

vector<Derived1>
vector<Derived2>
...
vector<DerivedN>

and then manually loop over each container each time you wanted to
process each element. This quickly becomes tedious. With this library I
can have a single container in my program and get approximately the same
speed.

So overall my impression is very positive. This is great work. I do have
a few things that I think needs careful considerations.

A. OO programming and copyability of classes is not something that
everybody is going to like. The normal thing is to derive from
boost::noncopyable to get rid of a bunch of undesired behaviors. Would
it be possible for the library to pick up a private or protected
copy-constructor when needed, possible via a friend declaration? I think
many uses of this library will also store pointers to the objects
in other collections, e.g. vector<const base*>, and as members so being
able to have your hierarchy non-copyable while allowing copyability
within the container is important IMO.

B. I miss range versions of the various iteration patterns so I can use
them directly with a range-based for. E.g. instead of

   for( auto i = c.begin<warrior>(); i != c.end<warrior>(); ++ i )

I should be able to say

   for( auto& : c.range<warrior>() );

C. I think we talked about an make_overload() function to devirtualize
uses of base_collection.  Do we have such a beast in boost already? If
not, it might be a worth providing it with this library.

D. Inside the segments you store std::vector. I could easily imagine the
need to store something else, say a flat_set or a container suitable for
non-movable, non-conpyable types (say a variant of deque).
Therefore I would love to have a second defaulted argument set to
std::vector. Now this is a template, template parameter which complicate
things ... so in a sense a policy with a nested template alias could do
the trick.

I also have other minor comments, but those given below.

>
> - What is your evaluation of the design?
>

It's excellent.

> - What is your evaluation of the implementation?

Very high quality, as usual.

> - What is your evaluation of the documentation?
>

Also high quality.

> - What is your evaluation of the potential usefulness of the library?

Very high.

> - Did you try to use the library? With what compiler? Did you have any
> problems?

No/No/No.

> - How much effort did you put into your evaluation? A glance? A quick
> reading? In-depth study?

About half a day.

> - Are you knowledgeable about the problem domain?
>

Somewhat.

> And most importantly:
>
> - Do you think the library should be accepted as a Boost library?
>

Yes.


*Specific comments*

A. why is subaddress() returning void* instead of T* ?

B. is the void* stuff like this

virtual range emplace( const_base_iterator p,void
(*emplf)(void*,void*),void* arg)
   {
     return range_from(s.emplace(iterator_from(p),emplf,arg));
}

  needed ? That is, is there no way to use real ans specific types?

C. Does value_holder<T>::data() need to return void*

D. Implementation of equality: do we need to be set like semantics
(two-pass)? Why don't you just delegate to the segments after checking
the size? I don't think the documentation specifies what the
implementation does.

E. should the test check (perhaps via static_assert) the conditions
under which move-construction and move-assignment is noexcept? (sorry if
this is already done). Specifically, if std::unordered_map has noexcept
for these, then you can guarantee the same for base_collection etc.

F. I understand the layout to be roughly
std::unordered_map<type_index,std::unique_ptr<segment>> ... this does
not quite match diagram on page 2 of documentation, that is, you are
missing an indirection.

G. tutorial: consider using override where appropriate


H. local iterator and undefined behavior?: does the code have an
assertion at least? Can we not get rid of undefined behavior of

   c.insert(c.begin(typeid(warrior)),juggernaut{11});

? That would surely be nice.

I. why emplace_pos/hint but not insert_hint/pos ?

J. why ==,!= but not <,>,>=, <=


K. clarify that

boost::poly_collection::for_each
   <warrior,juggernaut,goblin,std::string,window>( //restituted types
   c.begin(),c.end(),[&](const auto& x)


loops over all elements or only the specific.

L. It could be good to have a performance test of erase in the middle/front.

M. using the identifier "concept" may require change in the near future?

O. I wouldn't mind seeing separate graphs for 0-300 elements.

P. clarify test are without use of reserve

Q. is there no normal container size()/empty() ? docs say e.g.

   cc.size(index)
   cc.size<U>()

   but then later when we see the full base_collection interface they
are mentioned.

R. dumb question: is there any thing in your code that requires
std::is_polymorphic_v<Base> for base_collection ? If not, then perhaps
one should be able to avoid that and work completely with type
restitution ? In some sense, you could create a hierarchy where classes
a memcopyable and without a single virtual function.

S. The test are testing best possible scenario; a more realistic test
would be to copy pointers to an std::vector<base*>, shuffle it and run
update on that.


That's it. Great work :-).

kind regards

Thorsten


--
Best regards,
Thorsten Jørgen Ottosen, Ph.D.
Director of Research
+45 23308797
Dezide (www.dezide.com)













_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, May 2, 2017 at 4:46 PM, Ion Gaztañaga via Boost
<[hidden email]> wrote:
> The library can be found here:
> ...
>   https://github.com/joaquintides/poly_collection

The documentation looks beautiful and I was excited by the positive
review comments so I thought I would have a look. Unfortunately, I ran
into considerable difficulty trying to actually build the example and
poke around. There are no instructions on how to build that I could
find in the repository or the documentation.

First I tried cloning the repository and building:

$ cd ~/src
$ git clone [hidden email]:joaquintides/poly_collection
$ cd poly_collection/example
$ b2

Produced this output:
C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:111:
in load-parent from module project
error: Could not find parent for project at '.'
error: Did not find Jamfile.jam or Jamroot.jam in any parent directory.
C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:464:
in initialize from module project
C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:306:
in load-jamfile from module project
C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:64:
in load from module project
C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:145:
in project.find from module project
C:/Users/vinnie/lib/boost_1_64_0/tools/build/src\build-system.jam:535:
in load from module build-system
C:\Users\vinnie\lib\boost_1_64_0\tools\build\src/kernel\modules.jam:295:
in import from module modules
C:\Users\vinnie\lib\boost_1_64_0\tools\build\src/kernel/bootstrap.jam:139:
in boost-build from module
C:\Users\vinnie\lib\boost_1_64_0\boost-build.jam:17: in module scope from module

Now I remember some jibber jabber about putting projects in the live
Boost directory (a practice which I am not fond of). In the interest
of moving forward I tried it.

$ cd ~/lib/boost_1_64_0
$ git clone [hidden email]:joaquintides/poly_collection
$ cd poly_collection/example
$ b2

It got a little farther this time but still does not build:

Performing configuration checks

    - 32-bit                   : yes (cached)
    - arm                      : no  (cached)
    - mips1                    : no  (cached)
    - power                    : no  (cached)
    - sparc                    : no  (cached)
    - x86                      : yes (cached)
...patience...
...found 1033 targets...
...updating 23 targets...
compile-c-c++ ..\..\bin.v2\poly_collection\example\msvc-14.1\debug\threading-multi\algorithms.obj
algorithms.cpp
algorithms.cpp(12): fatal error C1083: Cannot open include file:
'boost/poly_collection/algorithm.hpp': No such file or directory

Did I mention I am using Microsoft Visual Studio on Windows?

Full disclosure, I have had this problem twice before when trying to
review libraries. In the past I have simply given up. But I want to
contribute something back since my own library will be reviewed soon.

Regards

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
On 5/10/2017 5:01 PM, Vinnie Falco via Boost wrote:

> On Tue, May 2, 2017 at 4:46 PM, Ion Gaztañaga via Boost
> <[hidden email]> wrote:
>> The library can be found here:
>> ...
>>    https://github.com/joaquintides/poly_collection
>
> The documentation looks beautiful and I was excited by the positive
> review comments so I thought I would have a look. Unfortunately, I ran
> into considerable difficulty trying to actually build the example and
> poke around. There are no instructions on how to build that I could
> find in the repository or the documentation.
>
> First I tried cloning the repository and building:
>
> $ cd ~/src
> $ git clone [hidden email]:joaquintides/poly_collection
> $ cd poly_collection/example
> $ b2
>
> Produced this output:
> C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:111:
> in load-parent from module project
> error: Could not find parent for project at '.'
> error: Did not find Jamfile.jam or Jamroot.jam in any parent directory.
> C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:464:
> in initialize from module project
> C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:306:
> in load-jamfile from module project
> C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:64:
> in load from module project
> C:/Users/vinnie/lib/boost_1_64_0/tools/build/src/build\project.jam:145:
> in project.find from module project
> C:/Users/vinnie/lib/boost_1_64_0/tools/build/src\build-system.jam:535:
> in load from module build-system
> C:\Users\vinnie\lib\boost_1_64_0\tools\build\src/kernel\modules.jam:295:
> in import from module modules
> C:\Users\vinnie\lib\boost_1_64_0\tools\build\src/kernel/bootstrap.jam:139:
> in boost-build from module
> C:\Users\vinnie\lib\boost_1_64_0\boost-build.jam:17: in module scope from module
>
> Now I remember some jibber jabber about putting projects in the live
> Boost directory (a practice which I am not fond of). In the interest
> of moving forward I tried it.
>
> $ cd ~/lib/boost_1_64_0
> $ git clone [hidden email]:joaquintides/poly_collection
> $ cd poly_collection/example
> $ b2
>
> It got a little farther this time but still does not build:
>
> Performing configuration checks
>
>      - 32-bit                   : yes (cached)
>      - arm                      : no  (cached)
>      - mips1                    : no  (cached)
>      - power                    : no  (cached)
>      - sparc                    : no  (cached)
>      - x86                      : yes (cached)
> ...patience...
> ...found 1033 targets...
> ...updating 23 targets...
> compile-c-c++ ..\..\bin.v2\poly_collection\example\msvc-14.1\debug\threading-multi\algorithms.obj
> algorithms.cpp
> algorithms.cpp(12): fatal error C1083: Cannot open include file:
> 'boost/poly_collection/algorithm.hpp': No such file or directory

1) Clone beneath the 'your_boost_tree_root/libs' directory
2) run 'b2 headers' from 'your_boost_tree_root' after you clone

>
> Did I mention I am using Microsoft Visual Studio on Windows?
>
> Full disclosure, I have had this problem twice before when trying to
> review libraries. In the past I have simply given up. But I want to
> contribute something back since my own library will be reviewed soon.
>
> Regards


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
On 5/10/17 2:46 PM, Edward Diener via Boost wrote:
  algorithms.cpp

>> algorithms.cpp(12): fatal error C1083: Cannot open include file:
>> 'boost/poly_collection/algorithm.hpp': No such file or directory
>
> 1) Clone beneath the 'your_boost_tree_root/libs' directory
> 2) run 'b2 headers' from 'your_boost_tree_root' after you clone
>
>>
>> Did I mention I am using Microsoft Visual Studio on Windows?
>>
>> Full disclosure, I have had this problem twice before when trying to
>> review libraries. In the past I have simply given up. But I want to
>> contribute something back since my own library will be reviewed soon.
>>

This is a common situation.  The boost directory structure doesn't play
nice with new libraries which are not already part of boost.  When I put
the safe numerics library into the incubator, I used path names for
includes so that one would not have to do the above procedure for the
new library.  This permitted one to build and run the tests and
examples, but of course created consternation of another sort.

Robert Ramey


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
AMDG

On 05/10/2017 03:46 PM, Edward Diener via Boost wrote:

> On 5/10/2017 5:01 PM, Vinnie Falco via Boost wrote:
>>
>> Now I remember some jibber jabber about putting projects in the live
>> Boost directory (a practice which I am not fond of). In the interest
>> of moving forward I tried it.
>>
>> $ cd ~/lib/boost_1_64_0
>> $ git clone [hidden email]:joaquintides/poly_collection
>> $ cd poly_collection/example
>> $ b2
>>
>> <snip>
>> algorithms.cpp
>> algorithms.cpp(12): fatal error C1083: Cannot open include file:
>> 'boost/poly_collection/algorithm.hpp': No such file or directory
>
> 1) Clone beneath the 'your_boost_tree_root/libs' directory

  Right.  It needs to go under libs/ because that's
where it will ultimately be put if it is accepted.

> 2) run 'b2 headers' from 'your_boost_tree_root' after you clone
>

  This is supposed to happen automatically, and it does
work for me with boost develop.

In Christ,
Steven Watanabe


_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Wed, May 10, 2017 at 2:46 PM, Edward Diener via Boost
<[hidden email]> wrote:
> 1) Clone beneath the 'your_boost_tree_root/libs' directory
> 2) run 'b2 headers' from 'your_boost_tree_root' after you clone

Okay, this worked and I was off to the races!

- What is your evaluation of the design?

It seems pretty straightforward. Given an explanation of the problem
the design of the collection feels natural and right. Most of the
container interface is standard boilerplate and so was immediately
familiar.

- What is your evaluation of the implementation?

I have not looked at the implementation, only the public interfaces.

- What is your evaluation of the documentation?

I was able to find everything I looked for. The diagrams look great
and really helped me understand the nature of the problem and how the
library solves it. The author makes container performance claims which
are then backed up with benchmarks across a variety of compilers and
systems.

My preference is for the style of documentation where exposition
precedes each member declaration (e.g.
http://www.boost.org/doc/libs/1_64_0/doc/html/boost_asio/reference/io_service.html)
but the presented style is also workable.

- What is your evaluation of the potential usefulness of the library?

These containers look quite useful, and I can think of at least a
couple of instances where I have rolled my own inferior solutions to
accomplish the same result.

- Did you try to use the library? With what compiler? Did you have any problems?

I had some trouble getting going using the Microsoft Visual C++
toolchain on Windows, but this had to do more with how the repository
needs to be cloned into a subtree of a boost library installation
rather than the proposed library itself.

I built the example using the compiler version 14.10.25017. Then I
added my own code to one of the example sources to get a feel for how
the library worked. I had no trouble at all writing correct code. My
attempts to abuse the library APIs were met with resistance from the
compiler in the form of compilation errors.

- How much effort did you put into your evaluation? A glance? A quick
reading? In-depth study?

I spent about an hour on it. I exercised mostly the
boost::base_collection container in my own program. I expect the
containers to work similarly. I have not used boost::any so I can't
comment on the any_collection. I focused on the "interesting" member
functions, the ones which operate differently from their standard
container counterparts. I assume the non-interesting functions act
with predictable results.

- Are you knowledgeable about the problem domain?

Yes. I have implemented many containers with conforming allocator
support of all varieties. I have used Boost.Intrusive heavily to
create complex containers and I have dabbled with some
Boost.MultiIndex.

- Do you think the library should be accepted as a Boost library?

This library is tightly focused on solving a specific, worthwhile
problem and does so in a way that feels natural and "boosty." So YES.

- Questions

Iterators returned by begin<T>() cannot be compared to iterators
returned by end(typeid(T)). Is this an oversight or a consequence of
the design? It seems to me they should be comparable.

- Other comments:

Iterators returned by begin<T>() produce a long, ugly compile error
when compared against iterators returned by begin<U>() or end<U>(). I
think the library could do better by producing a static_assert with a
helpful message. Its not a showstopper but a suggestion.

I echo the comments from other reviewers, a range<T>() function to
allow range-for iteration of a segment should be added at the earliest
convenience. I wouldn't hold up acceptance for it.

Thank you to the author for putting together a polished library!

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Hi Joaquin,

> On 10 May 2017, at 19:21, Joaquin M López Muñoz via Boost <[hidden email]> wrote:
>
> I have to think over the boost::function_collection given that it does't look as clear as
> I expected... As you correctly guess my aim was to have a single narrative, but seems
> like that's not working so well.

perhaps you can keep the narrative and just simplify the example a bit? I have nothing against the consistent narrative, it is a nice touch.

> Regarding the benefits of boost::function_collection<Signature> with respect to
> […]

Ok, I think I got it now, thanks :). As far as I am concerned, just adding this explanation to the tutorial would be enough.

>>> - Did you try to use the library? With what compiler? Did you have any problems?
>> I tried to compile the tests on a Mac with Apple-clang 8.0.0. It worked after I manually specified "cxxflags=-std=c++11" in the b2 call. By the way, it would be great if b2 used the highest standard that is available by default.
>
> It's weird because the Jamfile does have this flag already:
>
> https://github.com/joaquintides/poly_collection/blob/master/test/Jamfile.v2
>
> I guess the snapshot provided from the Incubator might be oldish and not
> include that... I can't check right now because blincubator.com is not responding :-(

It seems I got the right version, I checked out from github, not the incubator. My version of the Jamfile has these lines

project
    : requirements
      <toolset>gcc:<cxxflags>-std=c++11
      <toolset>clang:<cxxflags>-std=c++11
    ;

I am not a Boost.Build expert, but that looks ok to me. Nevertheless, when I run just ./b2 in the "tests" folder, it tries to compile but fails. When I run ./b2 cxxflags=-std=c++11, the tests compile fine.

Best regards,
Hans

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [review] Review of PolyCollection starts today (May 3rd)

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
El 10/05/2017 a las 22:33, Thorsten Ottosen via Boost escribió:

> Hi Joaquín (and Ion),
>
> [...]
>
> A. OO programming and copyability of classes is not something that
> everybody is going to like. The normal thing is to derive from
> boost::noncopyable to get rid of a bunch of undesired behaviors. Would
> it be possible for the library to pick up a private or protected
> copy-constructor when needed, possible via a friend declaration? I
> think many uses of this library will also store pointers to the objects
> in other collections, e.g. vector<const base*>, and as members so
> being able to have your hierarchy non-copyable while allowing
> copyability within the container is important IMO.

Technically, this is doable and, at first blush, simple, since all
element copying and
assignent is centralized in a single wrapper class:

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/value_holder.hpp

Another possibility is to use a copying traits class to let users
customize this point,
along the custom RTTI thing proposed by Ion. This looks to me even
cleaner and
more powerful.

I have no strong objection against including ths feature in whatever
form, but don't
particularly like it either --in the end, elements in a base_collection
must be copyable
(strictly speaking, moveable) and this will force users to change their
code by
writing the necessary ctors, so requiring an extra layer of boilerplate
(declaring
friends etc.) is just more hassle.

> B. I miss range versions of the various iteration patterns so I can
> use them directly with a range-based for. E.g. instead of
>
>   for( auto i = c.begin<warrior>(); i != c.end<warrior>(); ++ i )
>
> I should be able to say
>
>   for( auto& : c.range<warrior>() );

Yes, this is trivial and looks useful. At the risk of bikeshedding,
c.segment<warrior>()
might be a better name. Unfortunately there's no std naming practice here to
follow on, unless someone proves me wrong.

> C. I think we talked about an make_overload() function to devirtualize
> uses of base_collection.  Do we have such a beast in boost already? If
> not, it might be a worth providing it with this library.

hanna::overload is already here to the rescue:

https://lists.boost.org/Archives/boost/2017/03/233001.php

> D. Inside the segments you store std::vector. I could easily imagine
> the need to store something else, say a flat_set or a container
> suitable for non-movable, non-conpyable types (say a variant of deque).
> Therefore I would love to have a second defaulted argument set to
> std::vector. Now this is a template, template parameter which
> complicate things ... so in a sense a policy with a nested template
> alias could do the trick.

Of all your proposals this is the one I have most concerns about:
element contiguity is such
a key property that basically the whole library implicitly depends on
it, code- and
designwise. Policying this aspect away looks to me, in the absence of
cogent use cases,
an exercise in overdesign and a lot of work.

> [...]
>
>> - Do you think the library should be accepted as a Boost library?
>>
>
> Yes.

Thank you!

> *Specific comments*
>
> A. why is subaddress() returning void* instead of T* ?

Because only void* is needed: subaddress() is used in
poly_collection/detail/segment.hpp
as a client of the type-erased virtual interface defined in

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/segment_backend.hpp

> B. is the void* stuff like this
>
> virtual range emplace( const_base_iterator p,void
> (*emplf)(void*,void*),void* arg)
>   {
>     return range_from(s.emplace(iterator_from(p),emplf,arg));
> }
>
>  needed ? That is, is there no way to use real ans specific types?

This is connected to A, and in fact gives me the opportunity to comment
on one of
the more beautiful (IMHO) pieces of the lib :-)

The implementation of a typical emplace(args...) function just forwards the
args to the corresponding allocator construction call. The problem with
Boost.PolyCollection is that this construction happens inside a class
implementing
a virtual interface (the one mentioned before), and this interface can't
have
a member function template able to accept(args...). We have a type
erasure wall,
if you wish. So the workaround is to define a type-erased callback (the
emplf bit)
that is invoked from the virtual class implementation and provided by the
frontend of poly_collection, where the types of (args...) haven't been
erased.
Take a look at

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/poly_collection.hpp#L1015-L1034

> C. Does value_holder<T>::data() need to return void*

It could return T*, but that's not needed, as data() is used for
placement new.

> D. Implementation of equality: do we need to be set like semantics
> (two-pass)? Why don't you just delegate to the segments after checking
> the size? I don't think the documentation specifies what the
> implementation does.

We can't blindly delegate to the segments (if I'm getting your question
right) because
the order and associated types of the segments in x and y can differ: we
need to first
match them up, hence the two passes.

> E. should the test check (perhaps via static_assert) the conditions
> under which move-construction and move-assignment is noexcept? (sorry
> if this is already done). Specifically, if std::unordered_map has
> noexcept for these, then you can guarantee the same for
> base_collection etc.

I can't guarantee noexcept because whether an exception is thrown or not
depends
on the nature of the actual types in the container, which is runtime info.

> F. I understand the layout to be roughly
> std::unordered_map<type_index,std::unique_ptr<segment>> ... this does
> not quite match diagram on page 2 of documentation, that is, you are
> missing an indirection.

You're right, but the diagram is merely explanatory and I wouldn't want
to complicate
it to no avail. In fact, the segments are pointed to from what looks
like a small
vector of three elements (light blue), which is also not a correct
depiction of
a std::unordered_map internal structure. In the end, all these
indirections don't
affect performance in any significant way (workload is in the segments
proper).

> G. tutorial: consider using override where appropriate

Noted, will do.

>
> H. local iterator and undefined behavior?: does the code have an
> assertion at least? Can we not get rid of undefined behavior of
>
>   c.insert(c.begin(typeid(warrior)),juggernaut{11});
>
> ? That would surely be nice.

There's an assertion: take a look at

https://github.com/joaquintides/poly_collection/blob/master/include/boost/poly_collection/detail/poly_collection.hpp#L645-L658

> I. why emplace_pos/hint but not insert_hint/pos ?

This is insert(it,x). As for the naming, we have

emplace(args...)
emplace_hint(it,args...)
emplace_pos(local_it,args...)
insert(x)
insert(it,x)
insert(local_it,x)

which follows std conventions except for "emplace_pos". I had to made
that name up
because, in the standard, fixed position emplace only happens in
sequence containers
where the name is simply "emplace", but I can't use "emplace" as
emplace(it,args...)
would collide with emplace(args...) (which sequence containers don't have).

> J. why ==,!= but not <,>,>=, <=

Followed the interface of std::unordered_set. As segment order is
unspecified,
less-than comparison seems to make no sense.

> K. clarify that
>
> boost::poly_collection::for_each
>   <warrior,juggernaut,goblin,std::string,window>( //restituted types
>   c.begin(),c.end(),[&](const auto& x)
>
> loops over all elements or only the specific.

Loops over all elements, doing type restitution on those marked. Noted,
will clarify.

> L. It could be good to have a performance test of erase in the
> middle/front.

I can do that. Interpretation is going to be tricky, as the compared
data structures
differ wildly wrt erasure.

> M. using the identifier "concept" may require change in the near future?

Good observation! But, is "concept" not going to be a context-dependent
keyword
à la "final"?

> O. I wouldn't mind seeing separate graphs for 0-300 elements.

I can extend the graphs to cover that part, but I don't see the point of
using
this library for a handful of elements,don't you think?

> P. clarify test are without use of reserve

Noted, will do.

> Q. is there no normal container size()/empty() ? docs say e.g.
>
>   cc.size(index)
>   cc.size<U>()
>
>   but then later when we see the full base_collection interface they
> are mentioned.

There are normal size() and empty() member functions, they're described
in the
reference.

> R. dumb question: is there any thing in your code that requires
> std::is_polymorphic_v<Base> for base_collection ? If not, then perhaps
> one should be able to avoid that and work completely with type
> restitution ? In some sense, you could create a hierarchy where
> classes a memcopyable and without a single virtual function.

Actually, std::is_polymorphic_v is enforced on the reference but not
anywhere in the code.
I guess I preferred to be conservative here. As for working with type
restitution alone,
I think this scenario would be best served by a potential
variant_collection, which was
discussed at

https://lists.boost.org/Archives/boost/2017/03/233001.php

> S. The test are testing best possible scenario; a more realistic test
> would be to copy pointers to an std::vector<base*>, shuffle it and run
> update on that.

Sorry if I am not getting your question, but is this not the line
labeled "shuffled_ptr_vector"
in the plot?

> That's it. Great work :-).

What a thorough review! Thank you,

Joaquín M López Muñoz

_______________________________________________
Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost
123
Loading...