[review] [STLInterfaces] STLInterfaces review starts today

classic Classic list List threaded Threaded
23 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[review] [STLInterfaces] STLInterfaces review starts today

Boost - Dev mailing list
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
- Your knowledge of the problem domain
- Whether you believe the library should be accepted into Boost (be clear
about this)

In addition, you are strongly encouraged to answer the following questions:
- What is your evaluation of the library's
  * Design?
  * Implementation?
  * Documentation?
  * Tests?
  * Usefulness?
- Did you attempt to use the library? If so:
  * Which compiler(s)?
  * What was the experience? Any problems?
- How much effort did you put into your evaluation of the review?

STLInterfaces is a C++14 library targeting ISO standardization. The
following templates are provided, all C++20-friendly:

1. iterator_interface - a modern version of the iterator_facade and
iterator_adaptor parts of Boost.Iterator
2. view_interface - a pre-C++20 implementation of C++20's eponymous feature
3. container_interface - a tool to eliminate boilerplate when writing new
containers

We would appreciate answers to these library-specific questions:
- Do you like the name container_interface?  sequence_container_interface
is more precise, but seems a bit long.
- Would you use something like container_interface, but for associative
containers?  If so, should it assume a node-based interface, a la std::map
and std::set?  This assumption would preclude alternative associative
container-like types, such as flat_map.

Code: https://github.com/tzlaine/stl_interfaces
Docs: https://tzlaine.github.io/stl_interfaces/doc/html/index.html

Thanks,
Barrett

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

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

Boost - Dev mailing list
On Tue, Dec 10, 2019, 18:21 Barrett Adair <[hidden email]>
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
> - Your knowledge of the problem domain
> - Whether you believe the library should be accepted into Boost (be clear
> about this)
>
> In addition, you are strongly encouraged to answer the following questions:
> - What is your evaluation of the library's
>   * Design?
>   * Implementation?
>   * Documentation?
>   * Tests?
>   * Usefulness?
> - Did you attempt to use the library? If so:
>   * Which compiler(s)?
>   * What was the experience? Any problems?
> - How much effort did you put into your evaluation of the review?
>
> STLInterfaces is a C++14 library targeting ISO standardization. The
> following templates are provided, all C++20-friendly:
>
> 1. iterator_interface - a modern version of the iterator_facade and
> iterator_adaptor parts of Boost.Iterator
> 2. view_interface - a pre-C++20 implementation of C++20's eponymous feature
> 3. container_interface - a tool to eliminate boilerplate when writing new
> containers
>
> We would appreciate answers to these library-specific questions:
> - Do you like the name container_interface?  sequence_container_interface
> is more precise, but seems a bit long.
> - Would you use something like container_interface, but for associative
> containers?  If so, should it assume a node-based interface, a la std::map
> and std::set?  This assumption would preclude alternative associative
> container-like types, such as flat_map.
>
> Code: https://github.com/tzlaine/stl_interfaces
> Docs: https://tzlaine.github.io/stl_interfaces/doc/html/index.html
>

Only four days are left in this review, and there are no reviews or
comments received so far. Please consider reviewing this library if you
have any experience in this domain.

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

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

Boost - Dev mailing list
On 16/12/2019 17:46, Barrett Adair wrote:
>> STLInterfaces is a C++14 library targeting ISO standardization. The
>> following templates are provided, all C++20-friendly:
>>
>> 1. iterator_interface - a modern version of the iterator_facade and
>> iterator_adaptor parts of Boost.Iterator
>> 2. view_interface - a pre-C++20 implementation of C++20's eponymous feature
>> 3. container_interface - a tool to eliminate boilerplate when writing new
>> containers

I'm not sure the example in the Introduction is a great one to lead off
with.

The repeated_chars_iterator could be written like this in C++20, for
example:

     generator<char> repeated_chars(char const *first, size_t size,
size_t n)
     {
         while (true)
         {
             co_yield first[n++ % size];
         }
     }

(Or for more safety it could take a std::string_view.  Though both
implementations have a bug if iterated long enough that n overflows; I
just used a similar implementation as the example, although with
different types such that the repeated_chars_iterator produces UB and
possibly crashes and this merely skips some characters and otherwise
continues working.  Fixing that is left as an exercise for the reader.)

Granted, this is forward-only rather than random-access, and a generator
is not quite the same as an iterator (and has worse performance in
current implementations), but it better fits the stated goal.


There probably should also be some more discussion on how it interacts
with ranges, since AFAIK much of the need for custom iterators goes away
when you have arbitrarily filterable ranges.


I'm probably not going to have time to do a proper review, however --
it's already Christmas season.  [But I would like to see something like
this in general to be accepted.  We're still a long way from a C++20 world.]

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

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

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


> -----Original Message-----
> From: Boost <[hidden email]> On Behalf Of Barrett Adair via Boost
> Sent: 11 December 2019 00:21
> To: [hidden email]
> Cc: Barrett Adair <[hidden email]>
> Subject: [boost] [review] [STLInterfaces] STLInterfaces review starts today
>
> 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
> - Your knowledge of the problem domain
> - Whether you believe the library should be accepted into Boost (be clear about
> this)
>
> In addition, you are strongly encouraged to answer the following questions:
> - What is your evaluation of the library's
>   * Design?
>   * Implementation?
>   * Documentation?
>   * Tests?
>   * Usefulness?
> - Did you attempt to use the library? If so:
>   * Which compiler(s)?
>   * What was the experience? Any problems?
> - How much effort did you put into your evaluation of the review?
>
> STLInterfaces is a C++14 library targeting ISO standardization. The following
> templates are provided, all C++20-friendly:
>
> 1. iterator_interface - a modern version of the iterator_facade and iterator_adaptor
> parts of Boost.Iterator 2. view_interface - a pre-C++20 implementation of C++20's
> eponymous feature 3. container_interface - a tool to eliminate boilerplate when
> writing new containers
>
> We would appreciate answers to these library-specific questions:
> - Do you like the name container_interface?  sequence_container_interface is more
> precise, but seems a bit long.
> - Would you use something like container_interface, but for associative containers?
> If so, should it assume a node-based interface, a la std::map and std::set?  This
> assumption would preclude alternative associative container-like types, such as
> flat_map.
>
> Code: https://github.com/tzlaine/stl_interfaces
> Docs: https://tzlaine.github.io/stl_interfaces/doc/html/index.html

This proposal must be worth consideration, but are Boosters suffering from review fatigue after the long discussions on static_string (and about to be distracted by seasonal activities? 😉 )

Extend review period?

Paul




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

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

Boost - Dev mailing list
Why not extend the review period. I am not a regular reviewer, but I would
take a look at this one. I just haven't seen this proposal before now.

/Peter



On Mon, Dec 16, 2019 at 12:30 PM Paul A Bristow via Boost <
[hidden email]> wrote:

>
>
> > -----Original Message-----
> > From: Boost <[hidden email]> On Behalf Of Barrett Adair
> via Boost
> > Sent: 11 December 2019 00:21
> > To: [hidden email]
> > Cc: Barrett Adair <[hidden email]>
> > Subject: [boost] [review] [STLInterfaces] STLInterfaces review starts
> today
> >
> > 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
> > - Your knowledge of the problem domain
> > - Whether you believe the library should be accepted into Boost (be
> clear about
> > this)
> >
> > In addition, you are strongly encouraged to answer the following
> questions:
> > - What is your evaluation of the library's
> >   * Design?
> >   * Implementation?
> >   * Documentation?
> >   * Tests?
> >   * Usefulness?
> > - Did you attempt to use the library? If so:
> >   * Which compiler(s)?
> >   * What was the experience? Any problems?
> > - How much effort did you put into your evaluation of the review?
> >
> > STLInterfaces is a C++14 library targeting ISO standardization. The
> following
> > templates are provided, all C++20-friendly:
> >
> > 1. iterator_interface - a modern version of the iterator_facade and
> iterator_adaptor
> > parts of Boost.Iterator 2. view_interface - a pre-C++20 implementation
> of C++20's
> > eponymous feature 3. container_interface - a tool to eliminate
> boilerplate when
> > writing new containers
> >
> > We would appreciate answers to these library-specific questions:
> > - Do you like the name container_interface?
> sequence_container_interface is more
> > precise, but seems a bit long.
> > - Would you use something like container_interface, but for associative
> containers?
> > If so, should it assume a node-based interface, a la std::map and
> std::set?  This
> > assumption would preclude alternative associative container-like types,
> such as
> > flat_map.
> >
> > Code: https://github.com/tzlaine/stl_interfaces
> > Docs: https://tzlaine.github.io/stl_interfaces/doc/html/index.html
>
> This proposal must be worth consideration, but are Boosters suffering from
> review fatigue after the long discussions on static_string (and about to be
> distracted by seasonal activities? 😉 )
>
> Extend review period?
>
> Paul
>
>
>
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>

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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
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

> - 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.

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 &.
   - 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.

If there's a good reason for container_interface not to provide these,
then this reason should be documented.

>    * Implementation?

Didn't look at it.  I assume that it's fine, or that any issues with it
are too subtle for me to find.

>    * Documentation?

There are two types of tables with different columns in the
container_interface documentation: those that document user requirements
and those that document the functionality provided by
container_interface.  This can make reading difficult.  If I just want
to write a container I am only interested in the user requirements, so I
have to skip every other table.  If I am searching for specific
functions (using the text search functionality of my browser) to see if
container_interface provides it, I have to mentally switch between the
two table types.  It might be better to either merge the two table
types, or to group all tables of the same type together.

I think it should be clarified that although container_interface
provides no help in making containers allocator-aware, it can still be
used for writing allocator-aware containers.  It is simply up to the
user to provide all allocator-related functionality.  (At least I assume
that's the case?)

>    * Tests?

Didn't look.

>    * Usefulness?

Fairly useful.  I don't see myself using it often, it would definitely
be useful for the few times when I do need to write a custom container
or a custom iterator.

> - Did you attempt to use the library?

No.

> - Do you like the name container_interface?  sequence_container_interface
> is more precise, but seems a bit long.

Since I'm only going to be writing out the full name of the template
once per container, I think I prefer the longer, more precise name.

> - Would you use something like container_interface, but for associative
> containers?  If so, should it assume a node-based interface, a la std::map
> and std::set?  This assumption would preclude alternative associative
> container-like types, such as flat_map.

I can see the usefulness of such a template, although I have no
immediate use for it.  I would prefer that it not assume a node-based
interface.


--
Rainer Deyke ([hidden email])


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

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

Boost - Dev mailing list
On Mon, Dec 16, 2019 at 10:49 AM Rainer Deyke via Boost <
[hidden email]> 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

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

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

Boost - Dev mailing list
On 16.12.19 23:19, Zach Laine via Boost wrote:
>     - 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.

operator== is listed in Table 1.3, and in the User-Defined column of
Table 1.4.  The note in Table 1.8 also says that operator== is required
to be user-defined.  So if the library provides it, then the
documentation does not reflect this.


--
Rainer Deyke ([hidden email])


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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, Dec 10, 2019 at 4:21 PM Barrett Adair via Boost
<[hidden email]> wrote:
> The formal review of Zach Laine's STLInterfaces library begins now

I've asked a couple of folks I know to submit reviews, hopefully we
will see some new reviews shortly.

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

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

Boost - Dev mailing list
On Tue, Dec 17, 2019 at 6:52 AM Vinnie Falco via Boost <
[hidden email]> wrote:

> On Tue, Dec 10, 2019 at 4:21 PM Barrett Adair via Boost
> <[hidden email]> wrote:
> > The formal review of Zach Laine's STLInterfaces library begins now
>
> I've asked a couple of folks I know to submit reviews, hopefully we
> will see some new reviews shortly.
>

Thanks, Vinnie.

Zach

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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, Dec 17, 2019 at 12:39 AM Rainer Deyke via Boost <
[hidden email]> wrote:

> On 16.12.19 23:19, Zach Laine via Boost wrote:
> >     - 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.
>
> operator== is listed in Table 1.3, and in the User-Defined column of
> Table 1.4.  The note in Table 1.8 also says that operator== is required
> to be user-defined.  So if the library provides it, then the
> documentation does not reflect this.
>

Ah, I see.  Fortunately, that's only a doc problem.  For a container C with
value_type T, if operator==(T, T) is defined, then operator==(C, C) is
defined.  Similarly, if operator<(T, T) is defined, you get operator<(C, C)
from that.

If you provide a custom operator<(C, C), or use the one defined via
operator<(T, T) -> operator<(C, C), you get all the other relational
operators for C.  This was communicated poorly.  I'll fix that.

Zach

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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On 16/12/2019 19:46, I wrote:

> On 16/12/2019 17:46, Barrett Adair wrote:
>>> STLInterfaces is a C++14 library targeting ISO standardization. The
>>> following templates are provided, all C++20-friendly:
>>>
>>> 1. iterator_interface - a modern version of the iterator_facade and
>>> iterator_adaptor parts of Boost.Iterator
>>> 2. view_interface - a pre-C++20 implementation of C++20's eponymous
>>> feature
>>> 3. container_interface - a tool to eliminate boilerplate when writing
>>> new
>>> containers
>
> I'm probably not going to have time to do a proper review, however --
> it's already Christmas season.  [But I would like to see something like
> this in general to be accepted.  We're still a long way from a C++20
> world.]

After a bit more reading, a couple more points on the docs.

The iterator_interface tutorial specifies which members must be defined
in the derived class, but I fail to see anywhere where it indicates
which additional members will consequently be defined by the interface.

The container_interface tutorial does specify both, but it could
probably do with some additional headings to separate the tables (to aid
in finding the groups of operations that you're interested in),
especially since most of the tables themselves have identical captions.

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

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

Boost - Dev mailing list
On Tue, Dec 17, 2019 at 5:30 PM Gavin Lambert via Boost <
[hidden email]> wrote:

> On 16/12/2019 19:46, I wrote:
> > On 16/12/2019 17:46, Barrett Adair wrote:
> >>> STLInterfaces is a C++14 library targeting ISO standardization. The
> >>> following templates are provided, all C++20-friendly:
> >>>
> >>> 1. iterator_interface - a modern version of the iterator_facade and
> >>> iterator_adaptor parts of Boost.Iterator
> >>> 2. view_interface - a pre-C++20 implementation of C++20's eponymous
> >>> feature
> >>> 3. container_interface - a tool to eliminate boilerplate when writing
> >>> new
> >>> containers
> >
> > I'm probably not going to have time to do a proper review, however --
> > it's already Christmas season.  [But I would like to see something like
> > this in general to be accepted.  We're still a long way from a C++20
> > world.]
>
> After a bit more reading, a couple more points on the docs.
>
> The iterator_interface tutorial specifies which members must be defined
> in the derived class, but I fail to see anywhere where it indicates
> which additional members will consequently be defined by the interface.
>

I don't think this is necessary for the iterators, because you need all the
required user-provided operations for a given iterator category, and then
you get an iterator of that category -- in other words, you get all the
operations.  The container docs differ because there are multiple sets of
container requirements, and the mapping is not nearly so simple.


> The container_interface tutorial does specify both, but it could
> probably do with some additional headings to separate the tables (to aid
> in finding the groups of operations that you're interested in),
> especially since most of the tables themselves have identical captions.
>

That's a good idea.  I'll do that.

Zach

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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Howdy,
For this review I've really only tried out the iterator_interface part. I'm sorry I can't provide feedback on the container_interface nor view_interface, as I just don't have the free cycles.


> - Your name

Hadriel Kaplan


> - Your knowledge of the problem domain

I've been using boost's iterator_facade and adaptor for a long time; I've written my own custom iterators as well, of both traditional and proxy-types. The usual stuff.


> - Whether you believe the library should be accepted into Boost (be clear about this)

Definitely. It could use word-smithing and minor cleanup stuff, but presumably that happens later. (?)


> - What is your evaluation of the library's
>  * Design?

Good overall.


>  * Implementation?

Good. I've got some nitpicks, but honestly none are a big deal.

minor nitpicks:

1) I didn't use the proxy_iterator_interface one, only looked at the code. The proxy_arrow_result storing a local T value that it only returns a pointer to, means it's not optimal in some cases. Obviously for something like std::vector<bool> it won't matter, but some value-types are not so lightweight. On the other hand, it looks like we can specialize this for our type (or not even use proxy_arrow_result to begin with)?

2) Can the stuff in fwd.hpp be split out? I could be wrong, but I don't think most of it is needed/used by iterator_interface, and only used by the other two, correct? Reducing what gets included would be nice, for people who don't need the container/view stuff. The old boost iterator_facade/adaptor headers included so much that it drove some people to stop using them; for example see the comments here: https://github.com/facebook/folly/blob/master/folly/detail/Iterators.h

3) This lib hasn't been tested for C++20, and obviously can't be yet, but it makes assumptions that it can safely replace some of its internals with C++20's new ones. Things like concepts and ranges. I'm not sure that's advisable, until all the major compilers support them and can be tested. It would be a shame if people using this lib can't upgrade to C++20 because it breaks this lib, until they upgrade boost as well. (not a big deal, just more noting it)

4) The files use a macro to control doxygen output. Is this still required for boost libs? Doxygen has had the cond/endcond commands for a long, long time. It's kinda ugly to see these macros, and kinda silly that everyone's compiler will be evaluating them all the time. (I know that's super-nitpicky)

5) I assume all the _has_include(<stl2/ranges.hpp>) checks will be removed, since that's a thirdparty header?

6) This lib requires C++14, but I don't see a macro checking that anywhere and providing a simple error to the user.

7) I can't decide if the BOOST_STL_INTERFACES_STATIC_ASSERT_ITERATOR_TRAITS() thing is a good idea or a waste of time. I tried it, it passed. But of course it did, because it's just checking that I got the API that I told iterator_interface to create, no? This just seems like needless repetition to me. I mean if I screwed it up to begin with, I'll screw it up in this macro too, no? But maybe I'm missing something obvious. (could be - it's getting late)


>  * Documentation?

It's OK. I enjoyed some of the humor. :)

It's missing some info for things that are customization points.
For example base_reference() is shown in an example, but not really documented anywhere as far as I can tell.

The tables aren't as useful/explanatory as one would hope. I mean I knew what they meant, but I'm not sure someone newer to this area would. (but then again, that's what blogs/articles are for I guess)

I think it might also help if there were some explanation for what happens under-the-hood, in terms of what public methods get auto-generated from what, for which iterator types. For example, it's surprising that for a bidirectional iterator the user defines operator==(), but not for random-access iterator. (that particular case happens to be mentioned in a note, but you get my point)

There are also typos and such, but I presume that gets checked later.


>  * Tests?

Good. I copied them over and ran them as well. (but not the v2 tests, nor the ones for container_interface or view_interface)

I think it would be better if there were some tests for iterator_interface with non-copyable value-types. Testing it only for `int`s is cheating. :)

Also, random question: for the container_interface, did you try using it with the boost::fixed_string (or static_string, or whatever it's called)? I mean that _is_ a contiguous container, and it's in boost review too, so might it be a good test candidate? :)


>  * Usefulness?

Very useful. iterator_facade/adaptor were showing their age. It's time for something new and shiny.

I was happy that it reduced the number of functions we needed to implement in our current usages from 5 to 3.


> - Did you attempt to use the library?

Yup. I replaced three existing boost::iterator_facade usages in my company's code-base with this one, to try it out. Ran our unit tests with and without valgrind as well. But to be honest these weren't very complicated iterators.

I did _not_ try replacing boost::iterator_adaptor, but I hope to try that eventually.


>  * Which compiler(s)?

(old ones, so I could build my employer's codebase and run all their unit tests...)

1) gcc 7.3.1 with c++17
2) clang 5.0.0 with C++17, using gcc's libstdc++

Both using boost 1.55 (sorry!), with the stl_interfaces headers copied over manually, without using the CMake files in the github repo.
Neither compiler has the early Concepts support.


>  * What was the experience? Any problems?

Not really. The hardest part was figuring out the new model of what the derived had to define, compared to iterator_facade. (i.e., I was so used to the old model that it took me a bit to grok this one)


> - How much effort did you put into your evaluation of the review?

Only a few hours, maybe 4-5 total. Sorry, I wish I had more free time. :(


> - Do you like the name container_interface?  sequence_container_interface
> is more precise, but seems a bit long.

I'm not sure the length matters much for this use-case, as it's a one-time thing (generally), and not used by users of the actual container; and can anyway be aliased. (I mean, the "stl_interfaces" sub-namespace is annoying too, but it's not a big deal in my opinion)


> - Would you use something like container_interface, but for associative
>   containers?  If so, should it assume a node-based interface, a la std::map
>   and std::set?  This assumption would preclude alternative associative
>   container-like types, such as flat_map.

Yes I think so, depending on what it did... but for *unordered* ones (i.e., hash-maps/sets), rather than rb-trees.

I often find myself encapsulating them, and then having to write a lot of boilerplate public member functions. And several functions can be boiled down for hash-maps. For example count() and contains() can just use find().

I don't know why an unordered_container_interface wouldn't be do-able, including heterogenous lookups, for various hash-map/set flavors. (except, of course, that boost::unordered_map/set's API is different than std's for heterogeneous lookup; which is unfortunate)

-hadriel


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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
My mini-review will focus on the container_interface.

On Tue, Dec 10, 2019 at 7:21 PM Barrett Adair via Boost <
[hidden email]> wrote:

> - Your name
>

Krystian Stasiowski


> - Your knowledge of the problem domain
>

I enjoy implementing generic containers, and dabble in the subject.


> - Whether you believe the library should be accepted into Boost (be clear
> about this)
>

I vote to ACCEPT.


> - What is your evaluation of the library's
>   * Design?
>

Overall, its pretty solid. One notable thing that comes to mind is that
this should be made SFINAE friendly, ie disable certain overloads when the
required functions are not defined in the derived class. Additionally,
providing a destructor is a big no-no in my opinion, and could lead to
elements having their destructor called twice. Infact, the documentation
says that the user must provide a destructor that destroys all the elements
of the container, but the destructor provided by the container_interface
clears the container anyways, which leads to UB.


>   * Implementation?
>

This implementation is pretty solid.


>   * Documentation?
>

The documentation is _alright_. One thing I noticed is there is no cohesive
list of which functions are provided by the interface when others are
implemented by the user. It makes it quite a pain to navigate and even
confusing.


>   * Usefulness?
>

 I see this being useful for implementations of containers that don't have
to be extremely optimized. For example, the implementation of `clear`
provided by the implementation is not optimal for trivial types


> - Did you attempt to use the library? If so:
>   * Which compiler(s)?
>

MSVC.


>   * What was the experience? Any problems?
>

I would say that my experience with the library was quite good. To test it
out, I opted to rewrite Vinnie's boost::json::array container using
container_interface to see how much it would really simplify writing
containers. It mostly consisted of removing code as expected.


> - How much effort did you put into your evaluation of the review?
>

I spent about an hour and a half messing with the implementation.

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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
This my small review of STLInterfaces, and mainly iterator_interface

> - Your name
Andreas Wass

> - Your knowledge of the problem domain
I implement mainly iterators every once in a while, both at work and for
personal projects. I have mainly used iterator_interface but used
container_interface as well.

> - Whether you believe the library should be accepted into Boost (be clear
> about this)
Yes, it helps cutting down on the amount of boilerplate to write.

> In addition, you are strongly encouraged to answer the following questions:
> - What is your evaluation of the library's
>  * Design?
The design seems solid

>  * Implementation?
Looks solid as well, only had one issue with container_interface (see below).

>  * Documentation?
It is ok. As someone who isn't implementing containers all that often it would
be nice to have signatures as well as expressions listed in
the container_interface documentation

> - Did you attempt to use the library? If so:
>  * Which compiler(s)?
MSVC 2017 and GCC 9

>  * What was the experience? Any problems?
One problem with container_interface. See
https://github.com/tzlaine/stl_interfaces/issues/12

> - How much effort did you put into your evaluation of the review?
About two hours implementing iterators and a container

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

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

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

this is my mini review of STLInterfaces.

> - Your name

Daniela Engert

> - Your knowledge of the problem domain

I've written some iterators and containers in the past in our companies
in-house production code base. Currently I am about to write another
container with very particular features where this library would be very
handy by saving me from writing much boilerplate code.

> - Whether you believe the library should be accepted into Boost (be clear
> about this)

I vote to *CONDITIONALLY* ACCEPT the library after fixing the problem
that I will point out below

> In addition, you are strongly encouraged to answer the following questions:
> - What is your evaluation of the library's
>   * Design?
It looks solid and pretty much matches my expectations in this regard
>   * Implementation?

 - I'm not too fond of the BOOST_STL_INTERFACES_DOXYGEN clutter, but I
can live with it

- there are two instances of '#if 0' in container_interface.hpp which
should be removed

- I like it pretty much. In particular I want to point out that this is
one of the few Boost libraries that caused no compiler warnings in msvc
14.1 at warning level 4 from the get-go.

>   * Documentation?

It's ok. If one is familiar with may other Boost library documentations
she will eventually find what one is looking for. The uninitated might
find it hard to get into this stuff and dismiss the library as too
mysterious to him to be useful.

>   * Tests?
The library is lacking the integration into Boost's test framework.
Therefore I didn't run any tests. Sorry, no CMake on this computer.
>   * Usefulness?
High. I really want to use it.
> - Did you attempt to use the library? If so:
>   * Which compiler(s)?
VS2019 with msvc 19.2.4,  msvc 19.1.9, msvc 19.0.3
>   * What was the experience? Any problems?

I have been playing with the static_vector example as a starter because
it engages most parts of the library.

It doesn't compile with VS2015 (msvc 19.0.3) because of problems with
the 'noexcept()' operator related to function overloading. Even without
this problem it wouldn't compile because of the unconditional use of
__has_include which is not part of C++ 14!

----

Condition 1 for acceptance: make use of __has_include conditional. There
are conforming compilers that don't implement it. Or consider the
removal of the optional dependency on 3rd-party non-Boost libraries.

----

I had no problems with VS2017 (msvc 19.1.9)  in both debug and release
mode. I didn't check older versions of msvc 19.1.x

With the current version of VS2019 (i.e. 19.4) I experience the
following problems:

 - the static_vector example doesn't compile in C++ 14 mode because of
the missing include of <tuple> (std::tie in line 222 is undefined).
Alas, when compiled in C++ 17 mode all is fine.

 - the same source also compiles in C++ 2a mode when the compiler is
instructed not to reveal the true value of __cplusplus. But when it does
(by means of /Zc:__cplusplus) hell breaks lose. In this case the
compiler defines __cplusplus to 201705L and __cpp_lib_concepts to
201806L. This enables the compilation of the section of lines 557-695 in
iterator_interface.hpp and the section of lines 698-996 in
container_interface.hpp, thereby exposing

 -- the syntax error in line  731 of container_interface.hpp

 -- the use of entities from the empty namespace 'v2_dtl' in various
places in container_interface.hpp (first occurence in line 750)

 -- the compilation failure in iterator_interface lines 618-621, most
likely due to the problem that Andrzej has already pointed out in
https://github.com/tzlaine/stl_interfaces/issues/13

It doesn't matter if the v1 or the v2 interface is used here (for
obvious reasons).

----

Condition 2 for acceptance: address the syntactical problems

Condition 3 for acceptance: address the missing entities from namespace
'v2_dtl'

Condition 4 for acceptance: fix the compilation failures when C++ 20
concepts are enabled (like with msvc19.2.4 in C++ 2a mode and full
conformance turned on) and the respective code paths are engaged.

----

> - How much effort did you put into your evaluation of the review?
>
About 5 hours studying the documentation and implementation, and playing
with various versions of msvc to figure out all of the problems
mentioned above.

Ciao
   Dani




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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Mon, Dec 23, 2019 at 9:35 AM Andreas Wass via Boost <
[hidden email]> wrote:

> This my small review of STLInterfaces, and mainly iterator_interface
>
> > - Your name
> Andreas Wass
>

Thanks for reviewing, Andreas.

>  * What was the experience? Any problems?
> One problem with container_interface. See
> https://github.com/tzlaine/stl_interfaces/issues/12


Yes, I agree that the destructor implementation goes too far.  I'll
remove it.

Zach

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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Thu, Dec 19, 2019 at 1:12 PM Krystian Stasiowski via Boost <
[hidden email]> wrote:

> My mini-review will focus on the container_interface.
>
> On Tue, Dec 10, 2019 at 7:21 PM Barrett Adair via Boost <
> [hidden email]> wrote:
>
> > - Your name
> >
>
> Krystian Stasiowski
>

Thanks for the review, Krystian.


> > - What is your evaluation of the library's
> >   * Design?
> >
>
> Overall, its pretty solid. One notable thing that comes to mind is that
> this should be made SFINAE friendly, ie disable certain overloads when the
> required functions are not defined in the derived class.


This is the case, within the limits of the sequence container concept.
That is, if there is some operation (say, insert()) that is required by all
sequence containers, it would be inappropriate to SNIFAE-away such an
operation if the derived class does not support it.  To do so would mean
that you end up with a container_interface-derived type that is a sequence
container.  However, there are optional sets of requirements for sequence
containers (the reversible container requirements are one example).  For
those optional requirements, I believe that they are all SFINAE-friendly.
If you find a counterexample, that's a bug; please report it.


> Additionally,
> providing a destructor is a big no-no in my opinion, and could lead to
> elements having their destructor called twice. Infact, the documentation
> says that the user must provide a destructor that destroys all the elements
> of the container, but the destructor provided by the container_interface
> clears the container anyways, which leads to UB.
>

Agreed.  As stated in another thread, I'll remove the destructor
implementation.


>
> >   * Implementation?
> >
>
> This implementation is pretty solid.
>
>
> >   * Documentation?
> >
>
> The documentation is _alright_. One thing I noticed is there is no cohesive
> list of which functions are provided by the interface when others are
> implemented by the user. It makes it quite a pain to navigate and even
> confusing.
>

Could you point specifically to the part of the docs that you find hard to
follow?  There is already this sort of cohesive list in the case of
container_interface.  For the iterators, there is not.  In both cases, the
list of operations you end up with is well documented on cppreference,com
and eel.is, because it comes from the standard.

Zach

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

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

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
Zach Laine via Boost said:     (by the date of Thu, 26 Dec 2019 12:24:11 -0600)

> Yes, I agree that the destructor implementation goes too far.  I'll
> remove it.

why did you put it there in the first place?



--
# Janek Kozicki                              http://janek.kozicki.pl/

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