[libcxx-commits] [PATCH] D95983: [libc++] Further improve the contiguous-iterator story, and fix some bugs in D94807.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added inline comments.
This revision is now accepted and ready to land.


================
Comment at: libcxx/include/algorithm:1656
 
-template <class _Iter, bool = __is_cpp17_contiguous_iterator<_Iter>::value>
+template <class _Iter> class __wrap_iter;
+
----------------
Quuxplusone wrote:
> ldionne wrote:
> > I don't understand this change - didn't you work around the `constexpr` problem by *not* calling `unwrap_iter` on iterators when we're in a constexpr context?
> > 
> > Edit: Ah, I think you're trying to work around the *second* problem you pointed out, which is that you can't automatically re-create an arbitrary user-defined iterator from a raw pointer. Right? A solution for that could be to:
> > 
> > 1. Introduce an indirection like `__rewrap_iter_impl` with a nested `__apply` that creates an iterator from a raw pointer. That can be specialized as needed.
> > 2. Provide a default implementation for all iterators that are implicitly constructible from a pointer to their `value_type`.
> > 
> > That way, if the iterator isn't implicitly constructible from the pointer, a user like Chrome can still get the optimization by specializing the struct to tell libc++ how to get an iterator back from a pointer. And by default, it'll work for most iterators out of the box. Thoughts on that solution?
> > 
> > The only issue I can see is if implicit construction from a raw pointer is syntactically valid but doesn't do what we think it should do. I'm not sure how likely or dangerous that could be, but I would argue that if a contiguous iterator is constructible from a pointer but that constructor doesn't do the right thing, then you really voided your warranty with the Standard Library.
> > That way, if the iterator isn't implicitly constructible from the pointer, a user like Chrome can still get the optimization by specializing the struct to tell libc++ how to get an iterator back from a pointer.
> 
> I don't think we need `__rewrap_iter` to be a customization point at all. A contiguous iterator (even a user-defined one) is always a random-access iterator too, so we can use `__first + (__result - _VSTD::__unwrap_iter(__first))` as I indicated in my Phab comment a few lines above this. (If `__unwrap_iter` is a no-op, meaning that `_OrigIter` wasn't a contiguous iterator, just return `__result` unmodified.)
> 
> > I would argue that if a contiguous iterator is constructible from a pointer but that constructor doesn't do the right thing, then you really voided your warranty with the Standard Library.
> 
> I strongly agree in fantasy-everything-as-it-should-be-land, but I'm less sure in actual-C++-land. Remember this is the language that invented `std::pointer_traits<It>::to_address(it)` and `std::pointer_traits<It>::pointer_to(ptr)` instead of just using `static_cast<T*>(it)` and `static_cast<It>(ptr)`.
The issue with this approach is that it requires the unwrapped iterator to be reachable from the original iterator. That works for the algorithms we want to apply it to, but I could imagine other circumstances where that doesn't work, say if we're allocating a new range.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95983