[libcxx-commits] [PATCH] D138413: [libc++] Enable segmented iterator optimizations for join_view::iterator

[Louis Dionne via Phabricator via libcxx-commits]

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

This is a neat optimization! I do have some requests for changes, but once those are addressed I think we're on the right track.



================
Comment at: libcxx/include/__iterator/iterator_with_data.h:21
+
+template <class _Iterator, class _Data>
+class __iterator_with_data {
----------------
This is definitely more complicated, but we need to make this an actual iterator.


================
Comment at: libcxx/include/__ranges/join_view.h:72-73
 
-    template<bool> struct __iterator;
-    template<bool> struct __sentinel;
 
----------------
The reason why you're making this change is that otherwise, you can't specialize 

```
template <class ???>
struct __segmented_iterator_traits<???>
```

Naively, one would want to write

```
template <class _View, bool _Const>
struct __segmented_iterator_traits<ranges::join_view<_View>::__iterator<_Const>>
```

but obviously that doesn't work because one cannot specialize on a dependent type, which `__iterator` is. Technically, it would be possible to use a `enable_if` that checks for a nested member of the type (e.g. `__is_join_view_iterator`), but that's not great and it introduces a SFINAE path whenever we instantiate these traits, which is bad for compile-times.

The other option is what you've done here: define the iterator and sentinel types outside of the class. I think that's much better, however it's an ABI break. Fortunately we don't care about this for now because we're still shipping ranges experimentally, but I think we should make this change for all of our existing (and upcoming) views right now to future-proof them.

CC @var-const @huixie90 @Mordante for awareness


================
Comment at: libcxx/include/__ranges/join_view.h:272-273
 
+    _LIBCPP_HIDE_FROM_ABI constexpr __join_view_iterator(_Parent* __parent, _Outer __outer, _Inner __inner)
+      : __outer_(std::move(__outer)), __inner_(std::move(__inner)), __parent_(__parent) {}
+
----------------
Can this constructor be private? We don't want to expose non-standard APIs, and this iterator type is public, even though its name isn't public.


================
Comment at: libcxx/include/__ranges/join_view.h:403-404
 
+template <class _View, bool _Const>
+struct __segmented_iterator_traits<ranges::__join_view_iterator<_View, _Const>> {
+  using _Iterator = ranges::__join_view_iterator<_View, _Const>;
----------------
This would avoid forming a `__segmented_iterator_traits<It>` specialization when it would be nonsensical.


================
Comment at: libcxx/include/__ranges/join_view.h:405
+struct __segmented_iterator_traits<ranges::__join_view_iterator<_View, _Const>> {
+  using _Iterator = ranges::__join_view_iterator<_View, _Const>;
+
----------------
There's too much iterator-y stuff going on, let's be explicit about this.


================
Comment at: libcxx/include/__ranges/join_view.h:428-430
+    if (!__iter.__inner_.has_value())
+      return ranges::end(*--__iter.__outer_);
+    return *__iter.__inner_;
----------------
Suggestion? I was thrown off by the `*__iter.__inner_;`, I thought you were dereferencing the iterator instead of the optional.


================
Comment at: libcxx/test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.pass.cpp:124
+template <class Iter, class Sent>
+constexpr void test_join_view() {
+  auto to_subranges = std::views::transform([](auto& vec) {
----------------
I am worried about this test file becoming a mess if we start putting tests for all the segmented iterators here. Could we perhaps extract them to `test/std/algorithms/alg.modifying.operations/alg.copy/ranges.copy.segmented.pass.cpp` instead, along with the `deque` tests?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138413