Re: [review] [STLInterfaces] STLInterfaces review starts today

From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2019-12-16 22:19:55

On Mon, Dec 16, 2019 at 10:49 AM Rainer Deyke via Boost <
boost_at_[hidden]> wrote:

> On 11.12.19 01:21, Barrett Adair via Boost wrote:
> > Dear Boost,
> >
> > The formal review of Zach Laine's STLInterfaces library begins now, and
> > will run through December 19. Please participate in this review if you
> can.
> > To submit a review, please reply to this email with the following
> > information:
> > - Your name
>
> Rainer Deyke
>

Thanks for the review, Rainer!

> > - Your knowledge of the problem domain
>
> I'm hardly an expert on containers or iterators, but I've written a
> STL-compatible container or two.
>
> > - Whether you believe the library should be accepted into Boost (be clear
> > about this)
>
> Yes, it should be included.
>
> > In addition, you are strongly encouraged to answer the following
> questions:
> > - What is your evaluation of the library's
> >    * Design?
>
> Seems fine.  I have a few nits to pick, but nothing that would hold up
> the inclusion of the library in Boost.
>
> Nit 1: I don't like 'bool Contiguous = discontiguous'.  If a symbolic
> name like discontiguous is provided, then it should be part of an enum
> and not a plain boolean.  This prevents the name from being misused in
> other contexts.
>

I understand this completely, and usually I do exactly this, for exactly
this reason.  However, in this case, the Contiguous template param is a
hack that bridges us to C++20 code, which knows how to figure this out in a
standard way.  To make an enum for this implies a primacy that this
template parameter does not deserve.  That being said, if others find the
bool objectionable too, I'm not opposed to changing it.

> Nit 2: It seems to me that, with a bit of effort, container_interface
> could do much more to make containers easier to write.  For example:
>    - X::iterator can be inferred from begin().
>    - X::reference is just X::value_type &.
>

These cannot be provided by the CRTP base in the general case, because they
may be used in the body of the derived class, and they will not be visible
early enough if they come from the base class.  I'll document this.

   - operators < and == can be provided, since the requirements of these
> functions already suggest a default implementation.  The user can still
> override the provided implementation if a more efficient implementation
> is possible.  If it is for some reason desirable to create a container
> without operator <, an additional policy argument could be added to
> container_interface.
>

These are all provided; I think you just missed them.

Zach