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

[Joe Loser via Phabricator via libcxx-commits]

jloser added inline comments.


================
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) {
----------------
Quuxplusone wrote:
> 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.
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++`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108575