- add the from_range_t constructors and the related deduction guides;
- add the insert_range/assign_range/etc. member functions.
(Note: this patch is split from https://reviews.llvm.org/D142335)
Paths
| Differential D149827
[libc++][ranges] Implement the changes to `deque` from P1206 (`ranges::to`): ClosedPublic Authored by var-const on May 4 2023, 2:19 AM.
Details
Summary
(Note: this patch is split from https://reviews.llvm.org/D142335)
Diff Detail
Event Timelineldionne added inline comments.
Comment Actions Rebase on main (now that the vector patch is committed, the delta is significantly reduced). var-const added inline comments.
var-const marked an inline comment as done. Comment ActionsFeedback
Comment Actions LGTM w/ comments. In particular it would be nice to confirm that this doesn't introduce a regression in the existing deque methods (I don't think it would, but it's simple to confirm). Thanks!
This revision is now accepted and ready to land.May 12 2023, 12:26 PM var-const added inline comments.
Comment Actions A note in case it every comes in handy later: while working on this patch, I noticed that deque tests are significantly (5-10x) slower compared to the equivalent vector tests when compiled by GCC; with Clang, the difference is almost negligible. This is a preexisting issue -- running the test suite for insert_range while using the old insert function that takes two iterators on main shows a similarly large performance difference when compiling with GCC. From a conversation with @ldionne, one explanation could be that when compiling under GCC, many functions get force-inlined which might affect deque more significantly (and negatively) than vector. In any case, this patch doesn't make the situation worse. Comment Actions Thanks for adding benchmarks! LGTM once CI passes, which I think will require a CMake update on the Windows bots or some other workaround. Comment Actions Note: the benchmarks have been moved to https://reviews.llvm.org/D150747 (because they fail on the CI under Windows and I don't want to block this patch on that investigation). Closed by commit rG43377cc4a6da: [libc++][ranges] Implement the changes to `deque` from P1206 (`ranges::to`): (authored by varconst <varconsteq@gmail.com>). · Explain WhyMay 17 2023, 12:49 AM This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 520602 libcxx/include/__algorithm/copy_n.h
libcxx/include/deque
libcxx/test/std/containers/sequences/deque/deque.cons/deduct.pass.cpp
libcxx/test/std/containers/sequences/deque/deque.cons/from_range.pass.cpp
libcxx/test/std/containers/sequences/deque/deque.modifiers/append_range.pass.cpp
libcxx/test/std/containers/sequences/deque/deque.modifiers/assign_range.pass.cpp
libcxx/test/std/containers/sequences/deque/deque.modifiers/insert_range.pass.cpp
libcxx/test/std/containers/sequences/deque/deque.modifiers/prepend_range.pass.cpp
|
I think this needs a LWG issue.std::copy_n and ranges::copy_n have the exact same specification (http://eel.is/c++draft/alg.copy#lib:copy_n), but std::copy_n seems to be expected not to increment the input iterator if it can avoid it. ranges::copy_n does increment the iterator. For something like istream_iterator, this means we will be extracting different numbers of elements from the underlying stream.
This feels similar to the views::take issue we discussed in Issaquah, it might be worth checking whether that issue did talk about copy_n as another case of the same problem.
But for certain, I think we'll break users if we start increment this here, so we shouldn't do it unless we're blessed by the standard explicitly.