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

Subject: Re: [boost] [review] Review of PolyCollection starts today (May 3rd)
From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2017-05-11 02:46:39

On Wed, May 10, 2017 at 2:46 PM, Edward Diener via Boost
<boost_at_[hidden]> 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!