[libcxx-commits] [PATCH] D149827: [libc++][ranges] Implement the changes to `deque` from P1206 (`ranges::to`):

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/copy_n.h:42
         *__result = *__first;
+        ++__first;
         ++__result;
----------------
var-const wrote:
> ldionne wrote:
> > 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.
> Digging through the list of LWG issues, this has been raised before ranges as http://wg21.link/lwg2471. Ranges do make the issue more noticeable, though, because now the discrepancy is not just between `copy_n` and `uninitialized_copy_n`, but also `copy_n` and `ranges::copy_n`. It doesn't look like there's been any update on the issue, though.
> 
> @jwakely Were there any updates on this issue? I seem to remember this issue being brought up in Issaquah, but I couldn't find anything in LWG notes, so I might be confusing this with something. I can file a new LWG issue to talk specifically about the discrepancy between `copy_n` and `ranges::copy_n` -- do you think it makes sense?
@ldionne I ended up doing two versions of `assign_with_size` -- one for random access iterators and one for everything else. The random-access iterator version is essentially the previous implementation that uses `std::copy` (with all the potential optimizations). The other version essentially inlines `copy_n` (thankfully it's trivial without optimizations, which don't apply in this case) but keeps track of the incremented input iterator to avoid going through the input range twice. It's still more efficient than `assign_with_sentinel`: only one comparison per iteration when copying input elements into the existing capacity (the sentinel version has to compare both the input iterator and the deque iterator to their respective sentinels), and it can also allocate the necessary additional capacity at once.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149827