[libcxx-commits] [PATCH] D108575: [libcxx] Define insert_iterator::iter with ranges::iterator_t

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone requested changes to this revision.
Quuxplusone added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__iterator/insert_iterator.h:57
 
-    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, typename _Container::iterator __i)
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 insert_iterator(_Container& __x, decltype(iter) __i)
         : container(_VSTD::addressof(__x)), iter(__i) {}
----------------
jloser wrote:
> @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?
`decltype(iter)` seems fine to me. (This is a good catch! The new test achieved its goal!)


================
Comment at: libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/types.pass.cpp:31
 
+#include <__ranges/access.h>
 #include <iterator>
----------------
This should be `#include <ranges>` if you need it at all; but you don't.


================
Comment at: libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/types.pass.cpp:79-96
+    struct WithoutIteratorTypeAlias {
+    private:
+      using Container = std::vector<int>;
+
+    public:
+      WithoutIteratorTypeAlias() = default;
+      WithoutIteratorTypeAlias(const WithoutIteratorTypeAlias&) = default;
----------------
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.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108575/new/

https://reviews.llvm.org/D108575