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

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

Quuxplusone commandeered this revision.
Quuxplusone edited reviewers, added: jloser; removed: Quuxplusone.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/insert_iterator.h:38-44
 protected:
     _Container* container;
-    typename _Container::iterator iter; // FIXME: `ranges::iterator_t<Container>` in C++20 mode
-public:
+#if _LIBCPP_STD_VER > 17 && !defined(_LIBCPP_HAS_NO_RANGES)
+    ranges::iterator_t<_Container> iter;
+#else
+    typename _Container::iterator iter;
+#endif
----------------
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) {}
```


================
Comment at: libcxx/test/std/iterators/predef.iterators/insert.iterators/insert.iterator/cxx20_iter_member.pass.cpp:19
+  using value_type = int;
+  int* begin() { return nullptr; }
+  constexpr int* insert(int* pos, int value) {
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108575