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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
Comment at: libcxx/include/__algorithm/nth_element.h:76
         {
-            _VSTD::__selection_sort<_Compare>(__first, __last, __comp);
+            std::__selection_sort<_AlgPolicy, _Compare>(__first, __last, __comp);
             return;
----------------
huixie90 wrote:
> question: does this algorithm have the "robust against copy comparator test"? right now the comparator is copied several times but I guess this is fine
Yes, it is verified by the `robust...` test. `_Compare` is either a reference type, or, in debug mode, a small wrapper class that contains a reference member to the actual comparator (this is why `_Compare` is used explicitly to instantiate those functions -- deduction would deduce a non-reference type). Thus in either case, what's being passed around is a reference.


================
Comment at: libcxx/include/__algorithm/pop_heap.h:39
   if (__len > 1) {
     value_type __top = std::move(*__first);  // create a hole at __first
+    _RandomAccessIterator __hole = std::__floyd_sift_down<_AlgPolicy, _CompRef>(__first, __comp_ref, __len);
----------------
huixie90 wrote:
> should this call `iter_move`?
Thanks for catching this, I missed it somehow.


================
Comment at: libcxx/include/__algorithm/sort.h:44
+  using _Comp = _CompT;
+  _Comp& __comp;
+
----------------
huixie90 wrote:
> I am slightly concerned about the reference member because if we are going to copy the comparator around we have risk of having dangling reference. but I guess the trick is that we never copy the comparators?
It's a bit complicated, but there is always a value at the top of the call stack (in `std::sort` or `ranges::sort`), and all the functions down the call stack pass around a reference to the original value. `_WrapAlgPolicy` follows the same approach.


================
Comment at: libcxx/include/__algorithm/sort.h:71
+template <class _Wrapped>
+struct _UnwrapAlgPolicy<_Wrapped, std::enable_if_t<!std::is_same<typename _Wrapped::_AlgPolicy, int>::value>> {
+  using _AlgPolicy = typename _Wrapped::_AlgPolicy;
----------------
huixie90 wrote:
> where does the `int` come from? How can a policy be an `int`?
The expression is meaningless -- I just need to refer to `_Wrapped::_AlgPolicy` in a SFINAE context so that this specialization is preferred when `_Wrapped` is a `_WrapAlgPolicy`.


================
Comment at: libcxx/test/support/test_iterators.h:881-882
 
+  // If `T` is a reference type, the default assignment operator will be deleted.
+  constexpr Proxy& operator=(const Proxy& rhs) {
+    data = rhs.data;
----------------
huixie90 wrote:
> var-const wrote:
> > huixie90 wrote:
> > > hmm.  the two overloads above should already cover this. see the test
> > > https://github.com/llvm/llvm-project/blob/8922adf646eead207fb367dace80bba2abfab2ad/libcxx/test/support/test.support/test_proxy.pass.cpp#L63
> > > 
> > > or did I missing something?
> > I don't want to spend too much time investigating this just because of the deadline coming soon (although it's a pretty interesting issue). The test fails when the assigned type is `const` (which currently isn't tested).
> > 
> > My (unconfirmed) suspicion is that the implicitly-generated default `operator=` is a better match when the type on the right side is `const Proxy&`. Choosing between a template and a non-template function, overload resolution prefers a non-template function if both are an equally good match; `const Proxy&` is an exact match for the signature of the default `operator=`.
> > 
> > Confusingly, however, this works for the move assignment operator, even though the same logic about it being a better match seems to apply. cppreference says that "A deleted implicitly-declared move assignment operator is ignored by overload resolution". Presumably this doesn't apply to a deleted implicitly-declared _copy_ assignment operator, which would explain the difference in behavior.
> > 
> hmm. I suspect the tests are relying on the implicit conversion from T to Proxy<T>. (my original assignment explicitly check if `rhs` is `Proxy` and your new one allows implicit conversions. The class wasn't designed for `Proxy<T> =  T` but if that makes the test easier it would be fine. I will take a look later and it shouldn't block your patch. 
Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129823