Zach Laine wrote:

On Sun, Aug 11, 2019 at 5:43 AM Peter Dimov via Boost wrote:

...

Zach Laine wrote:

...

https://tzlaine.github.io/iterator_facade

`reference` being `char const` in the introductory example doesn't feel correct. `char const` can't be returned from a function (such as `operator*`), top-level qualifiers aren't meaningful for `char`.

It is correct, as of C++20. Proxy iterators are first-class citizens, as of the ranges concepts work. I used that reference type on purpose, and I refer to that later in the docs. Also, this is copy-paste from Boost.Text (proposed), which uses that exact iterator with a "char const" reference type.

Thanks. You should however be aware that both gcc and clang warn on functions returning `const char` with -Wextra. :-)

Dear Zach, Dear Zach,

On 11. Aug 2019, at 18:00, Zach Laine via Boost wrote:

On Sun, Aug 11, 2019 at 10:52 AM Peter Dimov via Boost < boost@lists.boost.org> wrote:

...

Zach Laine wrote:

...

On Sun, Aug 11, 2019 at 5:43 AM Peter Dimov via Boost wrote:

...

Zach Laine wrote:

...

https://tzlaine.github.io/iterator_facade

`reference` being `char const` in the introductory example doesn't feel correct. `char const` can't be returned from a function (such as `operator*`), top-level qualifiers aren't meaningful for `char`.

It is correct, as of C++20. Proxy iterators are first-class citizens, as of the ranges concepts work. I used that reference type on purpose, and I refer to that later in the docs. Also, this is copy-paste from Boost.Text (proposed), which uses that exact iterator with a "char const" reference type.

Thanks. You should however be aware that both gcc and clang warn on functions returning `const char` with -Wextra. :-)

Huh. Good to know. I guess I should change it to char then.

+1 for the proposal to have a modern iterator_fascade. I use a private implementation of iterator_fascade in Boost.Histogram for a couple of reasons, but mostly because the old boost iterator library pulls in a lot of boost dependencies which I want to avoid. But code duplication is bad, so I am looking forward to switch to your library which only depends on the stdlib. I object to change the name of iterator_fascade. The established convention in Boost is to call a updated library X with the same purpose X2, see variant and variant2, coroutine and coroutine2, signal and signal2, etc. This library should follow this route and be called iterator2. I am by no means an expert of the iterator_fascade, but I spend some time to figure out how the old iterator_fascade works, and it has some subtle solutions to corner case situations. Therefore, I would recommended that you port the iterator test suite to your project (if you haven't done so already). Also, please add a modern iterator_adaptor. iterator_adaptors are awesome, because you need to implemented even less. Best regards, Hans

Some more thoughts from a first glance at the library: In https://tzlaine.github.io/iterator_facade/doc/html/boost_iteratorfacade__pro... you list the user supplied operations. The method names should exactly correspond to the existing ones used by boost.iterator, see https://www.boost.org/doc/libs/1_71_0/libs/iterator/doc/iterator_facade.html compare vs. distance_to equals vs. equal next vs. increment prev vs. decrement The reason is that the old names were likely already "optimised" by previous reviews and you should make it easy for people to port existing code to iterator2. Switching from iterator to iterator2 should be as easy changing the included header for simple cases. Best regards, Hans

On Mon, Aug 12, 2019 at 8:38 AM Phil Endecott via Boost < boost@lists.boost.org> wrote:

Hans Dembinski wrote:

...

compare vs. distance_to

This reminds me of an issue that I've found with the existing iterator_facade: sometimes I can magnitude-compare the iterators, but not report a distance.

For example, a simple filter iterator can quickly tell whether A is before or after B by comparing the underlying iterators, but it cannot find the distance without applying its filter predicate to all the intermediate elements.

Currently, operator< etc. in the facade are implemented by calling the user's distance_to which is inefficient in this case. Your change of name to compare made me wonder if it is now expected only to return -1/0/+1, but that doesn't seem to be the case.

I think this is out of scope for the library, simply because the library is for making models of the C++20 iterator concepts. An iterator like you describe is not one of those -- an iterator that is not random access and yet has operator<() may be useful to you in some situations, but it is not useful for communicating capabilities to standard algorithms. Also, it's easy for you to bolt this on yourself if it's useful to you.

The other thing that I've found tedious to do with the current iterator_facade is implementing both iterator and const_iterator. The pattern I tend to use is:

template <bool is_const> class iterator_base: public boost::iterator_facade< ......... > ......

using iterator = iterator_base<false>; using const_iterator = iterator_base<true>;

I don't know if there is anything that iterator_facade could do to make this easier.

I've struggled with this too. I have not found a good way to deal with this. I think the best we can do is something like what you show above. One day reflection and/or "metaclass"-style source generation may be the way to do this. Zach

On Tue, Aug 13, 2019 at 7:18 AM Phil Endecott via Boost < boost@lists.boost.org> wrote:

Zach Laine wrote:

...

On Mon, Aug 12, 2019 at 8:38 AM Phil Endecott via Boost < boost@lists.boost.org> wrote:

...

Hans Dembinski wrote:

...

compare vs. distance_to

This reminds me of an issue that I've found with the existing iterator_facade: sometimes I can magnitude-compare the iterators, but not report a distance.

For example, a simple filter iterator can quickly tell whether A is before or after B by comparing the underlying iterators, but it cannot find the distance without applying its filter predicate to all the intermediate elements.

Currently, operator< etc. in the facade are implemented by calling the user's distance_to which is inefficient in this case. Your change of name to compare made me wonder if it is now expected only to return -1/0/+1, but that doesn't seem to be the case.

I think this is out of scope for the library, simply because the library is for making models of the C++20 iterator concepts. An iterator like you describe is not one of those -- an iterator that is not random access and yet has operator<() may be useful to you in some situations, but it is not useful for communicating capabilities to standard algorithms.

I disagree. If I try to use std::sort() or std::map<> with these iterators, they don't (IIUC) check the iterator category, but rather they just need operator< (or std::less?) to be defined.

std::map is a read herring. It does not need operator< on iterators, only on keys. As for std::sort, the existence of operator< on iterators is not enough to make this work. For instance, sort is allowed to use offsets and operator[] on your iterator, and yet there's no such thing. These concepts exist for a reason, and I'm not interested in subverting them.

Yes I can add them myself, but adding all of <, <=, >, >= is, pre-spaceship, tedious and a little error-prone - exactly the sort of boilerplate code that iterator_facade is trying to help eliminate.

Right, but in the cases you mention you only need operator<, right? Moreover, all the requirements on each concept are actually checked in the std::ranges algorithms. Only having operator< does not help you use algorithms that require a random_access_iterator does not work if the algorithm only uses operator<; the concept-checking machinery will still error out if you don't meet the full concept. Zach

On Sun, Aug 11, 2019 at 10:22 AM Zach Laine wrote:

On Sun, Aug 11, 2019 at 9:54 AM Francesco Guerrieri via Boost < boost@lists.boost.org> wrote:

...

Hi, just to express my interest in this. I already have a legacy project where this could be useful and if it were to land in boost (or in the standard) it would be great. Thanks for doing this.

Do you think that the code is already useable, or is there something "big" left to do? I hope to give it a try in some days and of course will report back to you if there is anything relevant or useful.

I consider it to be ready, as-is. There may be bugs lurking that need to be fixed, but I don't know of any. I can write every kind of iterator I've ever needed to write (which is a lot). This includes all the iterator categories, plus proxy versions of each, and even weird stuff like back_insert_iterator.

Yeah, about this statement. I meant it yesterday when I made it. Then I started investigating what it would take to make something like a range_facade, as was suggested earlier in this thread. That lead me to discover std::view_interface, which I previously had never even heard of: http://eel.is/c++draft/view.interface It's pretty similar in concept to what iterator_facade does, but it does it using concept-based SFINAE on the members of std::view_interface, which leads to a small, easy-to-understand library interface, and a lot less library code. It also allows the user to specify operations with their usual spellings, like operator*() instead of dereference(). It also does not require the access type, because you don't have to make functions with odd names that you want to hide. I like this a lot better. In fact, I like it so much better that I implemented something like it yesterday on a branch, renaming iterator_facade to iterator_interface: https://github.com/tzlaine/iterator_facade/tree/interface_alternative The one downside to this approach is that when you define operator++(), the iterator_interface cannot define operator++(int) for you automatically, because it gets hidden by the operator++() you just defined! Oh, C++, I wish I could quit you. Anyway, I worked around this for now by just keeping next() and prev(), and using operator*(), operator==(), operator+=() and operator-() for the other operations. I'm not super pleased with that, and I'm considering just using operator++() and operator--() instead of next() and prev(), and requiring the user to add "using operator++;" and/or "using operator--;". This has the nice property that if they forget, they'll get a compilation failure if anyone tries to use post-incr-/decrement. It has the bad property that it is one more thing for the user to remember. I'm certainly open to input here. So, there's your very large change. You might give both approaches a try and indicate which one feels more natural and why. Zach

On Aug 12, 2019, at 9:46 AM, Zach Laine via Boost wrote:

On Sun, Aug 11, 2019 at 10:22 AM Zach Laine wrote:

...

I consider it to be ready, as-is. There may be bugs lurking that need to be fixed, but I don't know of any. I can write every kind of iterator I've ever needed to write (which is a lot). This includes all the iterator categories, plus proxy versions of each, and even weird stuff like back_insert_iterator.

Have you considered an analogy of the Boost iterator_adaptor? That might reduce boilerplate in some useful cases. Cheers, Brook