[review] [STLInterfaces] STLInterfaces iterator_interface mini mini review

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

[review] [STLInterfaces] STLInterfaces iterator_interface mini mini review

Boost - Dev mailing list
On Wednesday, 11 December 2019 11:21:23 AM AEDT 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
 
Arthur Gruzauskas

> - Your knowledge of the problem domain

I've written a number of iterator interfaces over the years. Nothing fancy,
but have dipped my toes in iterator implementation.

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

ACCEPT, simply because I am using it happily.

but I have only used container_interface, not the rest.

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

simple interface, small learning curve, great value for effort.

A small number of methods to implement the essential concepts. A small enough
concept chunk to be easily manageable.

simple to use, just copied the code in the readme.md and adapted it.

>   * Implementation?

worked for me. I didn't examine the code much. I like its layout and it looks
clean to me.

>   * Documentation?

The readme.md with that simple example was enough for me to adapt the intro
sample iterator to my code.

I note the example has been criticised by Gavin Lambert a few posts ago. But I
love it as it made everything simple and clear.

Reading the main doc page filled in a few conceptual gaps, but that readme.md
gave me the overview that made the rest easy.

>   * Tests?

So far brief tests have all worked well. I did not exhaustively test the
iterators. They also worked fine in a couple STL algorithms. Again, I have only
dealt with iterator_interface.hpp

>   * Usefulness?

My experimental class would have stalled for some time if this easy iterator
interface had not been there. Encouraged,  I'm now looking at using it for
more complicated structures.

> - Did you attempt to use the library? If so:

>   * Which compiler(s)?

clang-8 for c++17 on debian linux.

>   * What was the experience? Any problems?

very straightforward.

a few small glitches, as I just grabbed  iterator_interface.hpp and fwd.hpp
and made small changes for it to work inside my project.

https://github.com/tzlaine/stl_interfaces/issues  describes two minor issues,
which could be well related to me just grabbing those 2 files only, outside the
boost structure that would normally sustain it.

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

Several hours reading, adapting and getting it working for me.

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

It doesn't clash with anything in my mental namespace, so I'm happy with the
name container_interface.

BTW, I have not used container_interface, I just saw it now and had a look. I
can see this simplifying some upcoming containers, and will certainly be
giving it a go.

view_interface is not something I have a need for currently.

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

My one small concern is in the required method:

constexpr auto operator-(repeated_chars_iterator other) const noexcept

instead of:

constexpr auto operator-(const repeated_chars_iterator& other) const noexcept

I may soon have a need for an iterator that holds a lot of state, and copying
that iterator for each subtraction makes me wonder about a performance penalty
in inner loops.

It is not a dealbreaker for this review, and I didn't examine Zach's tradeoffs
for doing it this way. I see many of my use cases where this approach will be
fine.

This is my first review, incomplete as it is, mainly motivated because no one
else has done one yet, and because I found iterator_interface useful and want
to encourage Zach for this lovely simplifying interface.

Thanks very much, Zach.


Arthur

 





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

Re: [review] [STLInterfaces] STLInterfaces iterator_interface mini mini review

Boost - Dev mailing list
On 17/12/2019 02:42, Arthur Gruzauskas wrote:
> ACCEPT, simply because I am using it happily.
>
> but I have only used container_interface, not the rest.

Point of confusion; later you said:

> So far brief tests have all worked well. I did not exhaustively test the
> iterators. They also worked fine in a couple STL algorithms. Again, I have only
> dealt with iterator_interface.hpp
[...]
> BTW, I have not used container_interface, I just saw it now and had a look. I
> can see this simplifying some upcoming containers, and will certainly be
> giving it a go.

Which is it?

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

Re: [review] [STLInterfaces] STLInterfaces iterator_interface mini mini review

Boost - Dev mailing list
On Wednesday, 18 December 2019 9:48:53 AM AEDT Gavin Lambert via Boost wrote:
> On 17/12/2019 02:42, Arthur Gruzauskas wrote:
> > ACCEPT, simply because I am using it happily.
> >
> > but I have only used container_interface, not the rest.

Oops, should be 'I have only used iterator_interface'

BTW, I know my review was simplistic, and only covered a small portion of the
proposed library.

I just did it to nag you more experienced folk into shrugging off very
understandable review fatigue.

>
> Point of confusion; later you said:
> > So far brief tests have all worked well. I did not exhaustively test the
> > iterators. They also worked fine in a couple STL algorithms. Again, I have
> > only dealt with iterator_interface.hpp
>
> [...]
>
> > BTW, I have not used container_interface, I just saw it now and had a
> > look. I can see this simplifying some upcoming containers, and will
> > certainly be giving it a go.
>
> Which is it?
>




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

Re: [review] [STLInterfaces] STLInterfaces iterator_interface mini mini review

Boost - Dev mailing list
In reply to this post by Boost - Dev mailing list
On Tue, Dec 17, 2019 at 9:59 AM Arthur Gruzauskas via Boost <
[hidden email]> wrote:

> On Wednesday, 11 December 2019 11:21:23 AM AEDT 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
>
> Arthur Gruzauskas
>

Thanks for the review, Arthur!

[snip]

> - Did you attempt to use the library? If so:
>
> >   * Which compiler(s)?
>
> clang-8 for c++17 on debian linux.
>
> >   * What was the experience? Any problems?
>
> very straightforward.
>
> a few small glitches, as I just grabbed  iterator_interface.hpp and
> fwd.hpp
> and made small changes for it to work inside my project.
>
> https://github.com/tzlaine/stl_interfaces/issues  describes two minor
> issues,
> which could be well related to me just grabbing those 2 files only,
> outside the
> boost structure that would normally sustain it.
>

For others following this, these issues are resolved for me locally, but
will not appear publicly during the review.  The issue is a missing
defined() in a couple of preprocessor expressions.

[snip]


> My one small concern is in the required method:
>
> constexpr auto operator-(repeated_chars_iterator other) const noexcept
>
> instead of:
>
> constexpr auto operator-(const repeated_chars_iterator& other) const
> noexcept
>
> I may soon have a need for an iterator that holds a lot of state, and
> copying
> that iterator for each subtraction makes me wonder about a performance
> penalty
> in inner loops.
>
> It is not a dealbreaker for this review, and I didn't examine Zach's
> tradeoffs
> for doing it this way. I see many of my use cases where this approach will
> be
> fine.
>

In short, I'm doing this for two reasons:

1) An iterator type is typically the size of a pointer or maybe two, and so
pass-by-value is usually more optimal than pass-by-reference.
2) All the STL interfaces that take iterators do it this way, and so making
the STLInterfaces usage pass-by-reference probably won't affect >= 90% of
your uses of a given iterator type.

Zach

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

Re: [review] [STLInterfaces] STLInterfaces iterator_interface mini mini review

Boost - Dev mailing list
On Wednesday, 18 December 2019 1:24:21 PM AEDT Zach Laine via Boost wrote:

[snip]

> > My one small concern is in the required method:
> >
> > constexpr auto operator-(repeated_chars_iterator other) const noexcept
> >
> > instead of:
> >
> > constexpr auto operator-(const repeated_chars_iterator& other) const
> > noexcept
> >
>
> In short, I'm doing this for two reasons:
>
> 1) An iterator type is typically the size of a pointer or maybe two, and so
> pass-by-value is usually more optimal than pass-by-reference.
> 2) All the STL interfaces that take iterators do it this way, and so making
> the STLInterfaces usage pass-by-reference probably won't affect >= 90% of
> your uses of a given iterator type.
>
> Zach
>

I had not thought about the need for an iterator to be lightweight before...

A revisiting of some standard STL algorithms implies copying iterators should
be quick so as to not impinge on the algorithms' performance.

Both 1) & 2) make sense.

I withdraw my small concern.

Arthur, who now only likes lightweight iterators




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