[libcxx-commits] [PATCH] D129823: [libc++][ranges] Make range algorithms support proxy iterators

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added a comment.

@rupprecht Thanks a lot for the reproducer! The issue here is the `varlen_element`'s heuristics for creating an independent copy of the underlying data. Currently, it's done in the move constructor, on the assumption that insertion sort inside `std::sort` would necessarily call the move constructor. However, this assumption is no longer true.

Previously, the temporary element would be created like this (simplified):

  value_type temp = std::move(*iter);

`*iter` returns a prvalue `varlen_element`, which `std::move` then casts to an rvalue reference, which results in the move constructor being invoked.

After the rewrite, the call to `std::move(*iter)` is replaced by an internal function `__iter_move` which emulates C++20's `iter_move`. `__iter_move` notices that `*iter` returns a temporary and thus also returns a value; attempting to cast it to an rvalue reference would result in a dangling reference:

  // simplified
  template <class _Iter, enable_if_t<IterDerefReturnsPrvalue<_Iter>>
  typename _Iter::value_type __iter_move(_Iter& __iter) {
    return *__iter;
  }

Due to mandatory copy elision, the move constructor is no longer invoked:

  value_type temp = __iter_move(*iter);

Instead, the result of calling `*iter` is stored right in `temp`, with no copying or moving involved.

Since there was no call to the move constructor, now two `varlen_element`s are shallow copies of each other, sharing a pointer to the same underlying data. Thus, the value of the temporary changes when the value of the original iterator from which it was created changes; that's how the repro case ends up with two `2439` elements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823