Fix basic_iterator for output cursors that don't provide post_increment().

[tahonermann]

• Added a post-increment operator overload to basic_iterator that returns a
  self reference for non-input cursors that don't provide post_increment().

• Added a test to validate that output iterators that hold state are correctly
  updated for post-increment dereference assignment.

• Updated a static_assert for ostream_iterator to reflect that post-increment
  now returns a self reference. This is consistent with the implementation of
  ostream_iterator in libstdc++.

[tahonermann]

These checks fail without the corresponding change to basic_iterator.hpp. The problem was that a copy of the output iterator was being returned for each post-increment operation. The dereference assignment then updated the copy leaving the wrapped iterator in 'i' left pointing to the first element of the vector.

[CaseyCarter]

These should work if you add a next member to output_cursor that increments i_, and do not perform the increment in write.

[tahonermann]

For my actual use case (otext_iterator: https://github.com/tahonermann/text_view/blob/master/include/text_view_detail/otext_iterator.hpp), it isn't possible to separate the write and the increment. This test was modelled on that use case, though I failed to express this requirement in this PR.

[tahonermann]

Actually, separating increment and dereference in this way results in undefined behavior for output iterators that wrap other output iterators if the post-increment operator doesn't return a self reference. In this case, the test is exercising an output iterator wrapping a random access iterator, so it isn't a problem. If the wrapped iterator was an actual output iterator, then the post-increment operation would have incremented the wrapped iterator held by i, the dereference operation would have dereferenced the wrapped iterator in the copy of i returned by the post-increment operation, and any subsequent increment operation on i would be undefined behavior. With the current design, the best way I see to avoid that undefined behavior is to implement post_increment to return a proxy that implements the dereference and assignment operators. I suppose that is ok, but since it is non-trivial to do, having a solution that allows a cursor to opt-in to having post-increment return a self reference seems desirable.

[CaseyCarter]

I suppose that is ok, but since it is non-trivial to do, having a solution that allows a cursor to opt-in to having post-increment return a self reference seems desirable.

You know you've written too many iterator hacks when this seems natural to you (https://github.com/CaseyCarter/text_view/tree/otext_iterator_fix):

    void next() noexcept {}
    auto post_increment() noexcept {
        class proxy {
            otext_cursor& self_;
        public:
            proxy(otext_cursor& self) noexcept : self_{self} {}

            proxy& operator*() noexcept {
                return *this;
            }
            proxy& operator=(const state_transition_type &stt) {
                self_.write(stt);
                return *this;
            }
            proxy& operator=(const character_type_t<encoding_type> &value) {
                self_.write(value);
                return *this;
            }
        };
        return proxy{*this};
    }

But I agree with your point: we've crossed a boundary into a place where it would be easier to avoid basic_iterator and simply write an iterator by hand. This design needs to do a better job with this use.

[tahonermann]

Ha! Nice, thanks! :)

[tahonermann]

The fix; post-increment must return a self reference rather than a copy for output iterators that hold state.

[tahonermann]

post-increment of an ostream_iterator now returns a self reference rather than a copy.

[CaseyCarter]

...which does not conform to N4620 [ostream.iterator.ops]/3.

[tahonermann]

I'm guessing N4620 is the next revision of the Ranges working draft? I don't see it published yet. I see what you are referring to in N4569 though.

[CaseyCarter]

The (almost totally undocumented) design for output cursors is that you either:

• Don't implement next, indicating that your output cursor is position-less/write-once. These iterators usually do all of their work in write, and do nothing on increment. basic_iterator will implement an output iterator that acts as its own proxy type, i.e., returns a self-reference from operator*, redirects assignments into the cursor's write member, and uses the no-requirement implementation of postfix ++ to effectively return a copy since prefix ++ has no effects. This variation is necessary to implement ostream_iterator and the insert iterators as specified in the TS.

• Implement next and don't implement post_increment. This is the "normal" mode of basic_iterator operation for Forward iterators: prefix ++ calls next, postfix ++ does the typical copy-increment-return-the-copy, operator* returns a proxy type that dispatches to write and/or read. This variation is typically not a good fit for purely output iterators.

• Implement both next and post_increment. Prefix ++ calls next. Postfix ++ calls post_increment and wraps its return value in an iterator if it returns a cursor, or returns it directly otherwise. (The existence of post_increment is an admission that basic_iterator is not smart enough to know which semantics you want for an input/output iterator post-increment operation. We give you a big hammer here to basically do whatever you want.)

• Implement post_increment and don't implement next, and see what happens. (The outcome is not designed, and should probably therefore be forbidden.)

[tahonermann]

Thanks for the detailed explanation regarding output curstors, next and post_increment. That was very informative.

[ericniebler]

Why is it called post_increment() instead of next(int)? And I don't yet understand why it's needed at all. For the output iterators I've seen one or both of next and write are sufficient. @tahonermann can you put the increment action in write?

But I agree with your point: we've crossed a boundary into a place where it would be easier to avoid basic_iterator and simply write an iterator by hand. This design needs to do a better job with this use.

I'm sure it can. basic_iterator already generates a proxy type for some cases. It can be made to generate one for this case as well.

[tahonermann]

@tahonermann can you put the increment action in write?

@ericniebler, for otext_cursor, the increment of the wrapped iterator necessarily needs to happen in write; that is what lead me to suggesting this change. The symptom I experienced was that, when an otext_iterator oiwrapped a forward-or-better iterator, post-increment/dereference/assign would call write on a copy of oi leaving oi holding a wrapped iterator that was not incremented. The next post-increment/dereference/assign then wrote to the same location as the previous operation.

[CaseyCarter]

Why is it called post_increment() instead of next(int)?

Because we don't have to differentiate prefix and postfix versions of the single name ++ - we can use different names and avoid the abomination that is (int). I'm not attached to the specific name post_increment, but I strongly dislike next(int).

And I don't yet understand why it's needed at all.

I added post_increment because there are a few sensible semantics for postfix increment of single-pass iterators (and some more exotic) that I wanted to be able to express:

• copy-increment-return-the-copy: move_iterator, istream_iterator
• return-a-copy: Ranges TS insert_iterators. (oops - this is fine for front_ and back_, but insert_iterator is broken in the same way as Tom's otext_iterator by this semantic: returning by value from postfix ++ means that the state of the copy is updated on write and then thrown out.)
• don't care (state is outside the iterator so increment does nothing and it doesn't matter to which copy we write): ostream_iterator, back_insert_iterator, front_insert_iterator
• return-a-reference (state in the iterator must be updated on write): otext_iterator, insert_iterator (broken in the TS: updates to the stored iterator are lost), ostreambuf_iterator (also broken in the TS: updates to the value of fail() can be lost)
• something ridiculously fancy: istreambuf_iterator (return a proxy with a copy of the current element and enough state to create a new iterator!) This is truly the intended use case for post_increment: do whatever craziness you like, and have basic_iterator get out of the way.

The current behavior for output cursors without next (return-a-copy) seems to be a mistake: return-a-reference seems a better fit for the known use cases. (postfix increment for iterator adaptors that handle both input and output iterators - common_iterator, counted_iterator - really doesn't have a chance.)

[CaseyCarter]

insert_iterator (broken in the TS: updates to the stored iterator are lost), ostreambuf_iterator (also broken in the TS: updates to the value of fail() can be lost)

Which is all on me: ericniebler/stl2#137. Apparently we were thinking about ostream_iterator, back_insert_iterator, and front_insert_iterator when we decided to change all the output iterator postfix increment operations to return a copy instead of a reference, and not about insert_iterator and ostreambuf_iterator.

[tahonermann]

My GitHub-fu is weak. I intended to submit a new pull request for commit 148eedb, but my new commit ended up added to this pull request.

This latest commit updated OutputIterator to add a requirement that the type returned by the post-increment operator be convertible to the iterator type. OutputIterator only requires WeaklyIncrementable since EqualityComparable is not required and WeaklyIncrementable doesn't require the post-increment conversion to iterator type because InputIterator doesn't. I just coded a direct requirement, I'm sure you'll want to code this differently.

The commit updates the test I added previously to add post_increment with a proxy and modifies one of the writes to the vector to use a copy initialized from a post-increment of the iterator. The constructor added to the mixin provides the necessary conversion. Remove the constructor and the test will fail to compile both because the static assert of OutputIterator fails and because the initialization of the copy fails due to the missing conversion constructor. Remove the additions to OutputIterator and you'll see the static assert disappear.

[tahonermann]

I pushed another commit to make the test I added actually test the change I originally intended it to test. Not sure what I was thinking adding implementations of next() and post_increment() in that intermediate commit.

[tahonermann]

Tom fail. This "fix" only works for cursors that don't implement next() or where next() has no side effects. Output iterators with internal state and next() implementations with side effects must produce a proxy object for post-increment. So, at a minimum, accepting this change would require adding a negative cursor::Next<C>() constraint; at which point, there is no need to dispatch to pre-increment operator++ and *this can be returned by reference directly, just as is done for the unconstrained pre-increment case.

constexpr basic_iterator& operator++(int) & noexcept
requires
    !cursor::Input<C>() &&
    !cursor::Next<C>() &&
    !cursor::PostIncrement<C>()
{
    return *this;
}

This suffices to implement stateful output iterators like otext_iterator that implicitly increment on write() and have no need to implement next() (or post_increment()).

This seems like a pretty narrow solution; I don't know how many output iterators this addresses. Thoughts?

[ericniebler]

This is feeling a bit like trying to make a dollar out of fifteen cents. An overly complex basic_iterator doesn't make the job of implementing an iterator simpler. I've said this before: if we want an iterator facade that can handle all possible input and output iterators, we should probably back up and rethink, since the current design is creaking under the load.

IMO we should defer all basic_iterator issues until after Kona. I'd like to see the open bugs in the Ranges TS addressed first.

[CaseyCarter]

IMO we should defer all basic_iterator issues until after Kona. I'd like to see the open bugs in the Ranges TS addressed first.

Yes, apologies to Tom. I had planned to address basic_iterator problems after completing my integration of the cmcstl2 basic_iterator into range-v3 and getting a fix for ericniebler/stl2#302 during the downtime before NB comments on the TS, that didn't quite work out to plan. I'll be back here sometime after Kona.

[tahonermann]

Yes, apologies to Tom.

Aw, man. I can't believe you aren't going to drop everything else you're doing for the betterment of the entire community just to focus on my pet issue.

I'll be back here sometime after Kona.

Sounds good. I'll keep my thinking cap on.