huixie90 requested changes to this revision.
huixie90 added inline comments.
================
Comment at: libcxx/include/__algorithm/inplace_merge.h:68
{
_VSTD::move(__first1, __last1, __result);
return;
----------------
should this call `std::ranges::move` for ranges policy? or `Algs<_AlgPolicy>::move` if that exists?
================
Comment at: libcxx/include/__algorithm/inplace_merge.h:185
// swap middle two partitions
__middle = _VSTD::rotate(__m1, __middle, __m2);
// __len12 and __len21 now have swapped meanings
----------------
I guess we want to `__rotate<AlgPolicy>` when it exists , perhaps add a todo here?
================
Comment at: libcxx/include/__algorithm/inplace_merge.h:202
+ __middle, __m2, __last, __comp, __len12, __len22, __buff, __buff_size);
// _VSTD::__inplace_merge<_Compare>(__first, __m1, __middle, __comp, __len11, __len21, __buff, __buff_size);
__last = __middle;
----------------
while you are here, perhaps consider remove these stale comments?
================
Comment at: libcxx/include/__algorithm/make_heap.h:28
inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX11
void __make_heap(_RandomAccessIterator __first, _RandomAccessIterator __last, _Compare& __comp) {
using _CompRef = typename __comp_ref_type<_Compare>::type;
----------------
we have two different patterns for these `__` functions.
1. the `__last` has a different template parameter type. and the `__` calls `next` to get the last iterator. the ranges counter part directly call it.
2. the `__last` has the same type and the ranges counter-part calls `next` then call into `__` function.
The problem is that when these two patterns co-exists, let's say `__a` takes two different types, and `__b` takes the same type. If `__a` needs to call `__b`, it could fail because `__b` expecting the same type.
I think we should make all algorithms consistent.
================
Comment at: libcxx/include/__algorithm/sort.h:142-166
+ is_same<decltype(*declval<_Iter>()), _Tp>::value>;
// Ensures that __c(*__x, *__y) is true by swapping *__x and *__y if necessary.
template <class _Compare, class _RandomAccessIterator>
inline _LIBCPP_HIDE_FROM_ABI void __cond_swap(_RandomAccessIterator __x, _RandomAccessIterator __y, _Compare __c) {
using value_type = typename iterator_traits<_RandomAccessIterator>::value_type;
bool __r = __c(*__x, *__y);
----------------
philnik wrote:
> Shouldn't this be `is_same<decltype(*declval<_Iter>()), _Tp&>::value`? Otherwise this would disable the optimization for (almost) all iterators, especially raw pointers. Also, could you qualify `declval`? While it's not strictly necessary it makes error messages a lot better.
I think you don't need to guard the `is_same<decltype(*declval<_Iter>()), _Tp>::value`
The algorithm is correct for proxy iterator, because of the correct use of `value_type`.
https://godbolt.org/z/c5Mh7aGxT
It'd only be would be wrong if it is using `auto`
https://godbolt.org/z/8xqzhPhve
Proxy Iterator needs to define `value_type` in the correct way to make these swap patterns work.
Normally these swap patterns need to be combined with `iter_move` (the triple-move swap), but here we are dealing with `is_arithmetic` types so moves are just copies
================
Comment at: libcxx/include/__algorithm/sort.h:337-340
+ ::new ((void*)__j2) value_type(_Ops::__iter_move(__i2));
__d.template __incr<value_type>();
for (--__j2; __i2 != __first2 && __comp(*__first1, *--__i2); --__j2)
+ *__j2 = _Ops::__iter_move(__i2);
----------------
nit: here we know that `__i2` is a pointer so `std::move(*__it2)` is actually ok? (to be confirmed)
================
Comment at: libcxx/include/__algorithm/sort.h:560-596
+extern template _LIBCPP_FUNC_VIS void __sort<_ClassicAlgPolicy, __less<char>&, char*>(char*, char*, __less<char>&);
#ifndef _LIBCPP_HAS_NO_WIDE_CHARACTERS
-extern template _LIBCPP_FUNC_VIS void __sort<__less<wchar_t>&, wchar_t*>(wchar_t*, wchar_t*, __less<wchar_t>&);
+extern template _LIBCPP_FUNC_VIS void __sort<_ClassicAlgPolicy, __less<wchar_t>&, wchar_t*>(wchar_t*, wchar_t*, __less<wchar_t>&);
#endif
-extern template _LIBCPP_FUNC_VIS void __sort<__less<signed char>&, signed char*>(signed char*, signed char*, __less<signed char>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<unsigned char>&, unsigned char*>(unsigned char*, unsigned char*, __less<unsigned char>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<short>&, short*>(short*, short*, __less<short>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<unsigned short>&, unsigned short*>(unsigned short*, unsigned short*, __less<unsigned short>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<int>&, int*>(int*, int*, __less<int>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<unsigned>&, unsigned*>(unsigned*, unsigned*, __less<unsigned>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<long>&, long*>(long*, long*, __less<long>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<unsigned long>&, unsigned long*>(unsigned long*, unsigned long*, __less<unsigned long>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<long long>&, long long*>(long long*, long long*, __less<long long>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<unsigned long long>&, unsigned long long*>(unsigned long long*, unsigned long long*, __less<unsigned long long>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<float>&, float*>(float*, float*, __less<float>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<double>&, double*>(double*, double*, __less<double>&);
-extern template _LIBCPP_FUNC_VIS void __sort<__less<long double>&, long double*>(long double*, long double*, __less<long double>&);
-
-extern template _LIBCPP_FUNC_VIS bool __insertion_sort_incomplete<__less<char>&, char*>(char*, char*, __less<char>&);
+extern template _LIBCPP_FUNC_VIS void __sort<_ClassicAlgPolicy, __less<signed char>&, signed char*>(signed char*, signed char*, __less<signed char>&);
+extern template _LIBCPP_FUNC_VIS void __sort<_ClassicAlgPolicy, __less<unsigned char>&, unsigned char*>(unsigned char*, unsigned char*, __less<unsigned char>&);
+extern template _LIBCPP_FUNC_VIS void __sort<_ClassicAlgPolicy, __less<short>&, short*>(short*, short*, __less<short>&);
----------------
hmm
================
Comment at: libcxx/src/algorithm.cpp:18-52
+template void __sort<_ClassicAlgPolicy, __less<wchar_t>&, wchar_t*>(wchar_t*, wchar_t*, __less<wchar_t>&);
#endif
-template void __sort<__less<signed char>&, signed char*>(signed char*, signed char*, __less<signed char>&);
-template void __sort<__less<unsigned char>&, unsigned char*>(unsigned char*, unsigned char*, __less<unsigned char>&);
-template void __sort<__less<short>&, short*>(short*, short*, __less<short>&);
-template void __sort<__less<unsigned short>&, unsigned short*>(unsigned short*, unsigned short*, __less<unsigned short>&);
-template void __sort<__less<int>&, int*>(int*, int*, __less<int>&);
-template void __sort<__less<unsigned>&, unsigned*>(unsigned*, unsigned*, __less<unsigned>&);
-template void __sort<__less<long>&, long*>(long*, long*, __less<long>&);
-template void __sort<__less<unsigned long>&, unsigned long*>(unsigned long*, unsigned long*, __less<unsigned long>&);
-template void __sort<__less<long long>&, long long*>(long long*, long long*, __less<long long>&);
-template void __sort<__less<unsigned long long>&, unsigned long long*>(unsigned long long*, unsigned long long*, __less<unsigned long long>&);
-template void __sort<__less<float>&, float*>(float*, float*, __less<float>&);
-template void __sort<__less<double>&, double*>(double*, double*, __less<double>&);
-template void __sort<__less<long double>&, long double*>(long double*, long double*, __less<long double>&);
+template void __sort<_ClassicAlgPolicy, __less<signed char>&, signed char*>(signed char*, signed char*, __less<signed char>&);
+template void __sort<_ClassicAlgPolicy, __less<unsigned char>&, unsigned char*>(unsigned char*, unsigned char*, __less<unsigned char>&);
+template void __sort<_ClassicAlgPolicy, __less<short>&, short*>(short*, short*, __less<short>&);
+template void __sort<_ClassicAlgPolicy, __less<unsigned short>&, unsigned short*>(unsigned short*, unsigned short*, __less<unsigned short>&);
+template void __sort<_ClassicAlgPolicy, __less<int>&, int*>(int*, int*, __less<int>&);
----------------
Can we introduce some new names like `__sort_with_al_policy`, which is the common sharable code between classic and ranges, and we keep `__sort` only for classic algorithm which calls the `__sort_with_al_policy` with classic policy so that these symbols are stable and have the same behaviour
================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_proxy_iterators.compile.pass.cpp:14
+//
+// Range algorithms should work with proxy iterators. For example, the implementations should use `iter_swap` (which is
+// a customization point) rather than plain `swap` (which might not work with certain valid iterators).
----------------
It is great to have a test for `iter_swap`. But this test won't cover the `iter_move`. If an algorithm doesn't use `iter_move` but use `std::move(*it)`, it would still compile (for copyable types) because `value_type x = std::move(*it)` would probably ends with copies.
For algorithms that do move stuff, we should probably test them with proxy iterator of move only types so that it will catch incorrect use of `std::move`
================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_proxy_iterators.compile.pass.cpp:28-45
+
+template <class Func, std::ranges::range Input, class ...Args>
+void test(Func&& func, Input& in, Args&& ...args) {
+ func(in.begin(), in.end(), std::forward<Args>(args)...);
+ func(in, std::forward<Args>(args)...);
+}
+
----------------
these tests are great and they can catch some problems immediately. and thanks for adding them.
However, since these tests don't actually verify the results of the algorithms, I think we should add additional tests in individual tests in the case-by-case manner.
For example, for sorting, we can test given a vector<int>, we can create a ProxyRange, sort the ProxyRange and verify the original vector is sorted
================
Comment at: libcxx/test/std/algorithms/ranges_robust_against_proxy_iterators.compile.pass.cpp:152
+ //test(std::ranges::prev_permutation, in, binary_pred);
+ //test(std::ranges::next_permutation, in, binary_pred);
+}
----------------
var-const wrote:
> Uninitialized memory algorithms don't work with `ProxyIterator` because it doesn't satisfy `nothrow-input-iterator` that requires the result of dereferencing the iterator to be the same as `iter_reference_t`. I'm not sure if it's possible to write a different valid proxy iterator that would satisfy that constraint but still require special handling in the implementation. @huixie90 What do you think?
The `nothrow-xxx-iterator` explicitly requires `std::same_as<std::remove_cvref_t<std::iter_reference_t<I>>, std::iter_value_t<I>>`, which basically means `value_type& == reference`, which rules out all the proxy iterators. It makes sense for writing to uninitialized memory because we do need the address of `*it` to be something that we can write to (not the proxies). So for most uninitialized algorithms, they are designed not to work with proxies. So we can't test them.
However, there are two exceptions. for `uninitialized_copy` and `uninitialized_move`, the inputs and outputs are separate. So in theory the inputs can be `ProxyRange` and the outputs can be a simple array which does model `nothrow-xxx-iterator`
================
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;
----------------
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?
================
Comment at: libcxx/test/support/test_iterators.h:895-896
+ // Helps compare e.g. `Proxy<int>` and `Proxy<int&>`. Note that the default equality comparison operator is deleted
+ // when `T` is a reference type.
+ template <class U>
----------------
thanks for adding this.
optional: consider add small test here as it gets more complicated now
test/support/test.support/test_proxy.pass.cpp
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129823/new/
https://reviews.llvm.org/D129823