[libcxx-commits] [PATCH] D110198: [libc++] Fix __wrap_iter to be a proper contiguous iterator.

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

Quuxplusone marked an inline comment as done.
Quuxplusone added inline comments.


================
Comment at: libcxx/include/__iterator/wrap_iter.h:293-296
+    _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR
+    static element_type *to_address(pointer __w) _NOEXCEPT {
+        return _VSTD::__to_address(__w.base());
+    }
----------------
@ldionne wrote:
> Can you please add tests in test/libcxx for pointer_to? It's not clear to me that it works at all.

You're right, it was totally busted — and `rebind` was busted worse! :P I'd just copied that code over without looking at it. It's gone now. I //think//, but am not sure, that this is still conforming. I don't think client code can conformingly rely on being able to look at `pointer_traits<vector<int>::iterator>`, let alone `pointer_traits<vector<int>::iterator>::rebind<const int>` or whatever. This `pointer_traits` is just an implementation detail that gets us the enabled `std::to_address` that we actually want to be client-visible.


================
Comment at: libcxx/include/__iterator/wrap_iter.h:287
+template <class _It>
+struct _LIBCPP_TEMPLATE_VIS pointer_traits<__wrap_iter<_It> >
+{
----------------
ldionne wrote:
> I think we should only specialize for `__wrap_iter<_Tp*>`, or perhaps for any `_It` that is a contiguous iterator. Otherwise, we're not guaranteed that it's legal to call `to_address(__w.base())` below. WDYT?
We already assume that `_It` must be a contiguous iterator; see line 283, also line 36. This was the outcome of that whole series of PRs among Zoe and myself that finally sorted out that //yes// `__wrap_iter` only ever actually wraps pointers, and it would break horribly if you ever manually tried to wrap anything that wasn't a pointer, so let's just stop pretending that it can.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110198