This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] Define insert_iterator::iter with ranges::iterator_t
ClosedPublic

Authored by Quuxplusone on Aug 23 2021, 12:52 PM.

Details

Summary

insert_iterator::iter member object is defined as Container::iterator but
the standard requires iter to be defined in terms of ranges::iterator_t as
of C++20. So, if in C++20 or later, define iter member object as
ranges::iterator_t.

Diff Detail

Event Timeline

jloser requested review of this revision.Aug 23 2021, 12:52 PM
jloser created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2021, 12:52 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
cjdb added a comment.Aug 23 2021, 12:57 PM

Please add a unit test, then I'll LGTM.

Also LGTM % lack of test. The new test should use a simple custom container that has a ranges::iterator_t but no nested ::iterator type. The only member function your custom container needs to provide is .insert.

libcxx/include/__iterator/insert_iterator.h
21–23

Our preferred style here is to #include <__ranges/access.h> unconditionally, alphabetized among the above headers. If !(_LIBCPP_STD_VER > 17), this #include will just be a no-op, and that's fine.

(Compare #include <__iterator/iterator.h> on line 14: that type is used on line 37 only if _LIBCPP_STD_VER <= 14 || !defined(_LIBCPP_ABI_NO_ITERATOR_BASES), but we don't guard line 14, only line 37.)

jloser updated this revision to Diff 368267.Aug 23 2021, 9:19 PM

Unconditionally include <__ranges/access.h>

jloser marked an inline comment as done.Aug 23 2021, 9:34 PM
ldionne requested changes to this revision.Aug 24 2021, 1:00 PM

Thanks for fixing this! As others have mentioned, please add a short test for this and I think it should be good to go.

libcxx/include/__iterator/insert_iterator.h
40–45

That should fix the CI issues you're seeing.

In the not-too-far future, we'll be able to drop _LIBCPP_HAS_NO_RANGES altogether, but for now we still support 1-2 compilers that don't implement concepts, and hence on which Ranges are not available.

This revision now requires changes to proceed.Aug 24 2021, 1:00 PM
jloser updated this revision to Diff 368547.Aug 24 2021, 9:26 PM

Add test for type without iterator type alias

jloser marked an inline comment as done.Aug 24 2021, 9:27 PM
jloser added inline comments.Aug 24 2021, 9:29 PM
libcxx/include/__iterator/insert_iterator.h
61

@cjdb @ldionne @Quuxplusone this is a drive-by fix so that the constructor of insert_iterator matches the iter type declared as the protected member. Do we like using decltype(iter) here so it's correct in both cases or would we prefer an #ifdef here like we do when defining the iter member?

jloser added a comment.EditedAug 24 2021, 9:31 PM

@ldionne @Quuxplusone @cjdb - added a test for a type without a nested iterator type alias to exercise the defining of iter member using std::ranges::iterator_t. Please me know what you think. Thanks!

Quuxplusone requested changes to this revision.Aug 24 2021, 9:55 PM
Quuxplusone added inline comments.
libcxx/include/__iterator/insert_iterator.h
61

decltype(iter) seems fine to me. (This is a good catch! The new test achieved its goal!)

libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/types.pass.cpp
31

This should be #include <ranges> if you need it at all; but you don't.

102–119

Lines 79–96 belong outside of the main function. Also, you're using vector instead of a custom container, you seem to be confusing container and value throughout, and you've given your "container" an insert method that returns an insert_iterator. Instead of all that, I suggest that you use something like the following code:

struct NoIteratorAlias {
    long data_[3] = {};
    using value_type = int;
    long *begin() { return nullptr; }
    constexpr long *insert(long *pos, int value) { *pos = value; return pos; }
};

#if TEST_STD_VER > 17
constexpr bool test_no_iterator_alias()
{
    NoIteratorAlias c;
    auto it = std::insert_iterator<NoIteratorAlias>(c, c.data_);
    *it++ = 1;
    *it++ = 2;
    assert(c.data_[0] == 1);
    assert(c.data_[1] == 2);
    assert(c.data_[2] == 0);
}
#endif // TEST_STD_VER > 17

int main(int, char**)
{
    test<std::vector<int> >();

#if TEST_STD_VER > 17
    test<NoIteratorAlias>();
    test_no_iterator_alias();
    static_assert(test_no_iterator_alias());
#endif // TEST_STD_VER > 17
}

This involves testing not just the existence of the typedefs but also the runtime behavior of the insert_iterator. I believe we currently have separate test files for those two things. So, either modify both test files, or create one new test file specifically for this feature. (The latter approach has the benefit that you can UNSUPPORTED that new test file pre-C++20 and/or when Ranges is not supported.)

This revision now requires changes to proceed.Aug 24 2021, 9:55 PM
ldionne accepted this revision as: ldionne.Aug 25 2021, 7:19 AM

LGTM with my and Arthur's comments applied, and CI passing.

Arthur, I'll let you give the final approval.

libcxx/include/__iterator/insert_iterator.h
44

You can remove this comment - the #if is so small that we can easily see the condition when looking at the #endif.

jloser updated this revision to Diff 368646.Aug 25 2021, 8:14 AM

Simplify test per Arthur's suggestions

jloser updated this revision to Diff 368655.Aug 25 2021, 8:35 AM

Remove #ifdef comment in insert_iterator.h

jloser added inline comments.Aug 25 2021, 8:37 AM
libcxx/include/__iterator/insert_iterator.h
44

Just removed it. I saw the comment on #endif in other files which is why I wrote that. Do we typically only write a comment if the #ifdef is long (i.e. the #endif is far from the starting #ifdef)?

jloser marked 2 inline comments as done.Aug 25 2021, 8:38 AM
jloser marked an inline comment as done.Aug 25 2021, 8:42 AM
jloser updated this revision to Diff 368656.Aug 25 2021, 8:43 AM

Remove #ifdef comment in test

jloser updated this revision to Diff 368801.Aug 25 2021, 9:20 PM

Avoid using decltype(iter) in insert_iterator constructor since this file may be included in C++03 mode and decltype specifier only works in C++11 or later.

jloser updated this revision to Diff 368859.Aug 26 2021, 6:16 AM

Split out testing iter member in C++20 into a seprate test file to avoid #ifdefs everywhere

Quuxplusone commandeered this revision.Aug 26 2021, 6:56 AM
Quuxplusone edited reviewers, added: jloser; removed: Quuxplusone.
Quuxplusone added inline comments.
libcxx/include/__iterator/insert_iterator.h
38–46

It would probably be better to introduce a private typedef __iterator to avoid repeating the ifdefs:

_LIBCPP_SUPPRESS_DEPRECATED_POP
#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
    typedef ranges::iterator_t<_Container> __iterator_type;
#else
    typedef typename _Container::iterator __iterator_type;
#endif
protected:
    _Container* container;
    __iterator_type iter;
~~~
    _LIBCPP_INLINE_VISIBILITY constexpr insert_iterator(_Container& __x, __iterator_type __i)
        : container(_VSTD::addressof(__x)), iter(__i) {}
libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
19 ↗(On Diff #368859)

I think we can omit the body of begin(), actually. Just long *begin(); should be good enough.
(I'd originally been giving it a body because I saw it was going to be called from test<NoIteratorAlias>(); but then I decided not to bother calling test<NoIteratorAlias>() after all.)
Also, yikes! You've lost the distinction between value_type=int and ranges::iterator_t<NoIteratorAlias>=long*, which, remember, is the entire point of this test.

But this has given me time to realize that we can do even better with the test. Leave value_type as int, but change the iterator type by making it float *begin() and float data_[4]...

Actually, let me just commandeer this.

Commandeered to update the test file. Also, update std::inserter for C++20 at the same time.

Flip the arguments to ASSERT_SAME_TYPE. Add a comment. NFC.

@ldionne, since I updated this significantly, I'm kicking it back to you for final approval. :)

jloser added inline comments.Aug 26 2021, 8:10 AM
libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
39 ↗(On Diff #368887)

Should we just use static_assert with std::is_same_v since this test file only supports C++20 anyway (meaning there isn't a need to route through ASSERT_SAME_TYPE)?

Quuxplusone marked an inline comment as done.Aug 26 2021, 8:19 AM
Quuxplusone added inline comments.
libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
39 ↗(On Diff #368887)

I think the question is not quite settled globally, but personally, I prefer ASSERT_SAME_TYPE because

  • it is shorter, and
  • it "always works" — doesn't require making any judgment calls about which C++ versions the test supports — and
  • it won't require refactoring five years down the road when the test gets extended to other C++ versions (this happens more often than you might think).

Hypothetically, if we were to go the "always use the latest and greatest possible, no software-engineering considerations" route, then we'd also have to consider static_assert with std::same_as ("since this test file only supports C++20 with concepts+ranges anyway").

jloser added inline comments.Aug 26 2021, 8:20 AM
libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
19 ↗(On Diff #368859)

Oops, sorry I mixed up the distinction between the value_type and the iter type from ranges::iterator_t when I split this into a separate test file. Also, good point about only needing a declaration for begin and not a definition now.

Thanks for taking over this work and cleaning it up. I appreciate your comments and patience with me on this review as I'm starting to get my feet wet in libc++.

cjdb accepted this revision.Aug 29 2021, 9:01 AM

This looks like it's ready to rock as-is. The only comment of mine that's blocking is the one addressing diagnostic concerns.

libcxx/include/__iterator/insert_iterator.h
61

Do we like using decltype(iter) here so it's correct in both cases or would we prefer an #ifdef here like we do when defining the iter member?

If this isn't public: have at it. If it is public: please use something that will produce the most meaningful diagnostic to the user.

libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
33 ↗(On Diff #368887)

Let's use constructible_from since it's also got a destructible sanity check.

39 ↗(On Diff #368887)

Unless you're intentionally directly testing the constructor on line 38, the way in which I'd write this is

std::same_as<std::insert_iterator<NoIteratorAlias>> auto it = std::insterer(c, c.data_);
ldionne accepted this revision.Sep 2 2021, 10:23 AM

LGTM with my comments applied.

libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
13 ↗(On Diff #368887)

Please add a comment explaining what this is testing. Something like Make sure we use ranges::iterator_t instead of Container::iterator in C++20 and above.

35 ↗(On Diff #368887)

More consistent with what we usually do.

This revision is now accepted and ready to land.Sep 2 2021, 10:23 AM
Quuxplusone marked 7 inline comments as done.Sep 2 2021, 1:11 PM
Quuxplusone added inline comments.
libcxx/include/__iterator/insert_iterator.h
61

This is how it looks after the PR:

x.cpp:7:39: error: no matching constructor for initialization of 'std::insert_iterator<decltype(v)>' (aka 'insert_iterator<vector<int>>')
    std::insert_iterator<decltype(v)> it(v, w.begin());
                                      ^  ~~~~~~~~~~~~
build2/include/c++/v1/__iterator/insert_iterator.h:58:61: note: candidate constructor not viable: no known conversion from 'std::vector<double>::iterator' (aka '__wrap_iter<double *>') to '__insert_iterator_iter_t<std::vector<int>>' (aka 'std::__wrap_iter<int *>') for 2nd argument
    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, __insert_iterator_iter_t<_Container> __i)
                                                            ^

compared to before:

x.cpp:7:39: error: no matching constructor for initialization of 'std::insert_iterator<decltype(v)>' (aka 'insert_iterator<vector<int>>')
    std::insert_iterator<decltype(v)> it(v, w.begin());
                                      ^  ~~~~~~~~~~~~
build/include/c++/v1/__iterator/insert_iterator.h:52:61: note: candidate constructor not viable: no known conversion from '__wrap_iter<std::vector<double>::pointer>' to '__wrap_iter<std::vector<int>::pointer>' for 2nd argument
    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, typename _Container::iterator __i)
                                                            ^

I call this "no significant difference in awfulness."

libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp
33 ↗(On Diff #368887)

This assertion is just sanity-checking that NoIteratorAlias is not constructible from int*, because if it were constructible from int*, then that would nerf the whole point of the test (which is to verify that insert_iterator doesn't accidentally try to construct it from int* instead of double*). Destructibility is irrelevant.