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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

LGTM except for the weird includes, and also I'd like to have a test that exercises `std::to_address` for a `__wrap_iter`. I suspect the `#include` confusion started as an attempt to add such a test?

I don't need to see this again unless it changes significantly. Thanks a bunch for looking into that!



================
Comment at: libcxx/include/__iterator/wrap_iter.h:287
+template <class _It>
+struct _LIBCPP_TEMPLATE_VIS pointer_traits<__wrap_iter<_It> >
+{
----------------
Quuxplusone wrote:
> 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.
Sorry, I meant to delete my comment before submitting the review. You're right, I remembered that after writing it.


================
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());
+    }
----------------
Quuxplusone wrote:
> @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.
Yeah.. I don't think that specialization of `pointer_traits` would be conforming for a user defined type, but we can do it cause we're the implementation.


================
Comment at: libcxx/test/std/utilities/memory/pointer.traits/pointer_to.pass.cpp:20-23
+#include <string>
+#include <string_view>
+#include <vector>
+
----------------
Why those includes?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110198