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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const marked an inline comment as done.
var-const added a comment.

Note: I finished the tests (at least in the amount that I had planned originally). I still need to fix the CI.



================
Comment at: libcxx/include/__ranges/to.h:98
+        __iterator_traits_detail::__cpp17_input_iterator<iterator_t<_Range>> &&
+        constructible_from<_Container, iterator_t<_Range>, sentinel_t<_Range>, _Args...>
+    ) {
----------------
ldionne wrote:
> Can you add tests that would fail if this didn't short-circuit properly? TBH I think those tests would fail right now.
Thanks for pointing this out! Added tests.


================
Comment at: libcxx/include/deque:604
+    : __map_(__pointer_allocator(__a)), __start_(0), __size_(0, __a) {
+    if constexpr (ranges::sized_range<_Range>) {
+      __append_n(ranges::begin(__range), ranges::size(__range));
----------------
ldionne wrote:
> In the case of `deque`, it's not clear to me that we gain much from using `__append_n`. In both cases, we end up performing `N / CHUNK_SIZE` allocations and then copying the whole input range into these allocations. Maybe worth measuring? I think looking at how `__add_back_capacity(size_type __n)` and `__add_back_capacity()` differ will provide some additional insights here.
> 
> Also, if we end up keeping this optimization, the `if constexpr` condition seems a bit off. What should we do for a `forward_range` that is not a `sized_range`? We'll have to determine whether we want to take the hit to compute `distance(rng)` and then call `__append_n`, or not.
I did a very quick benchmark:
```
template <class Container, class GenInputs>
void BM_ConstructFromRangeT(benchmark::State& st, Container, GenInputs gen) {
  auto in = gen(st.range(0));
  benchmark::DoNotOptimize(&in);
  while (st.KeepRunning()) {
    Container c(std::from_range, in);
    DoNotOptimizeData(c);
  }
}

BENCHMARK_CAPTURE(BM_ConstructFromRangeT,
  deque_int,
  std::deque<int>{},
  getRandomIntegerInputs<int>)->Arg(5140480 * 10);

BENCHMARK_CAPTURE(BM_ConstructFromRangeT,
  deque_string,
  std::deque<std::string>{},
  getRandomStringInputs)->Arg(1024 * 10);

BENCHMARK_MAIN();
```

I ran it with and without the optimization 5 times and took the average (going by the wall time but the difference with CPU time is negligible):
```
Test                              Time, ns
ints, with optimization        34045820
ints, no optimization          67872734
strings, with optimization     2057941
strings, no optimization       2000087
```

So for strings, the difference is negligible, presumably because the timings are dominated by copying (in fact, the optimized version is 3% slower, but I'd chalk it up to random fluctuations), but for ints the optimized version is 2x faster, making it worthwhile to keep.


================
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:
> 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. :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142335