Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
862126a
Fix basic_iterator for output cursors that don't provide post_increme…
tahonermann Dec 7, 2016
148eedb
Updated OutputIterator to require post increment to return a type con…
tahonermann Dec 9, 2016
1f1f69f
Removed implementations of next(), post_increment(), and the post inc…
tahonermann Jan 21, 2017
23c5990
Merge branch 'master' of https://github.com/tahonermann/cmcstl2
tahonermann Jan 23, 2017
a9481b2
Restrict basic_iterator's post-increment operator++ implementation th…
tahonermann Jan 24, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions include/stl2/detail/iterator/basic_iterator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,6 +726,15 @@ STL2_OPEN_NAMESPACE {
++*this;
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


constexpr decltype(auto) operator++(int) &
noexcept(noexcept(declval<C&>().post_increment()))
requires cursor::PostIncrement<C>()
Expand Down
55 changes: 54 additions & 1 deletion test/iterator/basic_iterator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,30 @@ std::ostream& operator<<(std::ostream& os, R&& rng) {
return os;
}

template <stl2::Iterator I>
class output_cursor {
public:
struct mixin : protected stl2::detail::ebo_box<output_cursor> {
mixin() = default;
using mixin::ebo_box::ebo_box;
};

output_cursor() = default;
constexpr output_cursor(I i) noexcept
: i_{i} {}

template <class T>
requires stl2::OutputIterator<I, const T&>()
constexpr void write(const T& t) noexcept {
*i_++ = t;
}

private:
I i_;
};
template <stl2::Iterator I>
using output_iterator = stl2::basic_iterator<output_cursor<I>>;

void test_fl() {
std::cout << "test_fl:\n";
::forward_list<int> list = {0, 1, 2, 3};
Expand Down Expand Up @@ -484,7 +508,7 @@ void test_counted() {
CHECK((one <= three));
CHECK(!(one >= three));
}
}
}

void test_always() {
std::cout << "test_always:\n";
Expand Down Expand Up @@ -598,6 +622,34 @@ void test_proxy_array() {
CHECK(a[3] == 1);
}

void test_output() {
std::cout << "test_output:\n";

auto vec = std::vector<int>{0,0,0};
auto wi = vec.begin();

using I = ::output_iterator<decltype(wi)>;
static_assert(stl2::models::WeaklyIncrementable<I>);
static_assert(!stl2::models::Incrementable<I>);
static_assert(!stl2::models::Decrementable<I>);
static_assert(!stl2::models::Readable<I>);
static_assert(stl2::models::Writable<I, int>);
static_assert(stl2::models::DefaultConstructible<I>);
static_assert(stl2::models::Copyable<I>);
static_assert(stl2::models::Semiregular<I>);
static_assert(stl2::models::Iterator<I>);
static_assert(!stl2::models::InputIterator<I>);
static_assert(stl2::models::OutputIterator<I, int>);

I i{wi};
*i++ = 1;
*i++ = 2;
*i++ = 3;
CHECK(vec[0] == 1);
CHECK(vec[1] == 2);
CHECK(vec[2] == 3);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ha! Nice, thanks! :)

}

int main() {
test_rv();
test_fl();
Expand All @@ -606,5 +658,6 @@ int main() {
test_always();
test_back_inserter();
test_proxy_array();
test_output();
return ::test_result();
}
2 changes: 1 addition & 1 deletion test/iterator/ostream_iterator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ int main() {
static_assert(models::Same<I&, decltype(*i)>);
static_assert(models::Same<I&, decltype(*i = 42)>);
static_assert(models::Same<I&, decltype(++i)>);
static_assert(models::Same<I, decltype(i++)>);
static_assert(models::Same<I&, decltype(i++)>);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


static_assert(noexcept(I{}));
static_assert(noexcept(I{ss}));
Expand Down