[libcxx-commits] [PATCH] D101404: [libcxx] Fix __wrap_iter to work with to_address.

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

Quuxplusone requested changes to this revision.
Quuxplusone added a comment.
This revision now requires changes to proceed.

If this PR is the last thing needed to unblock landing something for C++20, then OK ship it, I don't think it's introducing any //wrong// behavior. However, if we have time, I would like to see further investigation of how this patch fits into the general scheme of D94807 <https://reviews.llvm.org/D94807>, and how much of that scheme is still relevant this month. In particular:

- Can we make `__is_cpp17_contiguous_iterator` more similar to `std::contiguous_iterator`, perhaps by having `__is_cpp17_contiguous_iterator` look at whether `element_type` exists?

- Can we now remove the specialization of `__is_cpp17_contiguous_iterator` for `__wrap_iter`?

- Can we now remove the overload of `_VSTD::__to_address` for `__wrap_iter`? I think no, but I'm not sure (and it sure would be nice if we could).

- You're providing `element_type` unconditionally, which means that `__wrap_iter<It>` will be treated as pointer-like even when `It` is not a (fancy) pointer type itself; e.g. `__wrap_iter<std::list<int>::iterator>`. My impression is that we actually //only ever use `__wrap_iter` to wrap (fancy) pointers// — is this accurate? If it is not accurate, then more digging is warranted. If it is accurate, then we should document that invariant, and maybe even search-and-replace `_Iter` to `_Ptr` throughout `__wrap_iter`.

D101003 <https://reviews.llvm.org/D101003> is related. (I'm eliminating `span::iterator`'s dependence on `__wrap_iter<T*>`.)



================
Comment at: libcxx/include/iterator:1280
     typedef typename iterator_traits<iterator_type>::pointer           pointer;
+    typedef typename pointer_traits<pointer>::element_type             element_type;
     typedef typename iterator_traits<iterator_type>::reference         reference;
----------------
If my comments are correct, then this could+should use `pointer_traits<_Iter>` instead of `pointer_traits<pointer>`...
...and the `_If` on line 1284 is unconditionally true, and can be simplified.
Please try that simplification (at least locally) and see if it passes the test suite.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101404