[libcxx-commits] [PATCH] D142335: [libc++][ranges] Implement `ranges::to`.

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/to.h:71
+template <class>
+concept __always_false = false;
+
----------------
var-const wrote:
> @ldionne Do you think it's worthwhile to move this concept to a dedicated header?
Re `__always_false`:

I don't think so, unless we have similar uses for the same thing. I would move it to a separate header when/if we have other use cases.


================
Comment at: libcxx/include/deque:825
+      if constexpr (ranges::bidirectional_range<_Range>) {
+        auto __n = static_cast<size_type>(ranges::distance(__range));
+        return __insert_bidirectional(__position, ranges::begin(__range), ranges::end(__range), __n);
----------------
Here, you basically want to compute the distance, but also the last iterator (not sentinel) as you're doing it. Then you can pass down that information to `__insert_bidirectional` to avoid having to ever re-compute the end iterator, which would require re-walking the whole range.

It strikes me that

```
template< std::input_or_output_iterator I, std::sentinel_for<I> S >
constexpr void advance( I& i, S bound );
```

should really return `std::iter_difference_t` representing the number of times it incremented the iterator to get to the `bound`. Then you'd do something like:

```
auto __last = ranges::begin(__range);
auto __n = std::advance(__last, std::end(__range));
// now you have an iterator representation of your sentinel AND the knowledge of the size of the range, which you can pass down to the function
```

Edit: Scratch everything above. There's a good reason why `std::ranges::advance` doesn't return the `iter_difference_t`: it would prevent the assignment optimization from happening when the sentinel is an iterator. I guess I don't have a great solution for this then. If you can think of one, good, otherwise I guess we can settle for computing the distance twice in some cases.



================
Comment at: libcxx/include/deque:1292-1294
+        _Iterator __m = std::next(__f, size()); // TODO(varconst): NEEDS DISCUSSION
+        std::__copy<_ClassicAlgPolicy>(__f, __m, begin());
+        __append_with_size(__m, __n - size());
----------------
I think we want this here:

```
// no need to compute `__m` here anymore
auto __rest = std::__copy_n<_ClassicAlgPolicy>(__f, size(), begin()).first;
__append_with_size(__rest, __n - size());
```


================
Comment at: libcxx/include/deque:1297
     else
-        __erase_to_end(_VSTD::copy(__f, __l, begin()));
+        __erase_to_end(std::__copy<_ClassicAlgPolicy>(__f, __l, begin()).second);
 }
----------------
```
__erase_to_end(std::__copy_n<_ClassicAlgPolicy>(__f, __n, begin()).second);
```

And then you don't need to take `_Sentinel __l` as an input anymore, and that saves you from having to compute `std::next` above.


================
Comment at: libcxx/include/deque:1823
+deque<_Tp, _Allocator>::__insert_bidirectional(const_iterator __p, _BiIter __f, _Sentinel __sent, size_type __n) {
+    auto __l = std::ranges::next(__f, __sent);
     size_type __pos = __p - begin();
----------------
This does have the same problem that we're re-walking the whole input sequence again in some cases, let's see if there's a solution for that.


================
Comment at: libcxx/include/forward_list:873
+    _LIBCPP_HIDE_FROM_ABI
+    iterator insert_range_after(const_iterator __position, _Range&& __range) {
+      return __insert_after_with_sentinel(__position, ranges::begin(__range), ranges::end(__range));
----------------
You could consider moving this back below to minimize the diff, but I can see what you did here and I think it's fine too.


================
Comment at: libcxx/include/queue:478
+          class _Alloc,
+          class = __enable_if_t<__is_allocator<_Alloc>::value>>
+    queue(from_range_t, _Range&&, _Alloc)
----------------
var-const wrote:
> Not sure if this SFINAE is actually needed (I followed the existing pattern). Concepts and the `from_range_t` should work to distinguish these from existing CTAD "overloads", and there is no ambiguity between the 2 and 3 argument versions.
What does the spec say? I think there's blanket wording based on the name of the template parameters that mean that something called `Allocator` needs to be a valid allocator. IDK whether it applies to this specific CTAD. I think it is https://eel.is/c++draft/container.requirements#container.reqmts-69.

I suspect it is needed. If so, we almost certainly have tests for this in e.g. `std::vector`, and we should mirror that here.


================
Comment at: libcxx/test/std/ranges/range.utility/range.utility.conv/to.pass.cpp:48
+static_assert(!std::ranges::view<ContainerT>);
+static_assert(HasTo<ContainerT, InputRange>);
+static_assert(!HasTo<test_view<forward_iterator>, InputRange>);
----------------
var-const wrote:
> var-const wrote:
> > ldionne wrote:
> > > I think this is technically ill-formed according to the spec. I wonder whether it makes sense to test the exact way in which it is ill-formed as we do here (probably not).
> > > 
> > > Also note that if that's correct and this is ill-formed, then technically we could also be SFINAE friendly as a QOI matter. I don't think we should unless it's simple and other implementations are doing it as well.
> > Sorry, but at this point I need a reminder on why this is ill-formed.
> @ldionne Tagging this comment so that it doesn't get drowned out among the many other comments. :)
This comment was originally attached to

```
using ContainerT = int;
static_assert(!std::ranges::view<ContainerT>);
static_assert(HasTo<ContainerT, InputRange>);
```

but Phabricator kind of messed things up. I think https://eel.is/c++draft/range.utility.conv#to-2.3 makes that ill-formed. Personally, I feel like we could simply drop that test. The similar test for `HasTo<test_view<...>>` is relevant though, cause it tests the `requires (!view<C>)` on `ranges::to` (but we might want to add a comment on that test saying what we are testing).



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335