[libcxx-commits] [PATCH] D127194: [libc++] Implement ranges::is_permutation

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:36
 Read-only,clamp,Nikolas Klauser,`D126193 <https://llvm.org/D126193>`_,Under review
-Read-only,is_permutation,Not assigned,n/a,Not started
+Read-only,is_permutation,Nikolas Klauser,` <https://llvm.org/>`_,✅
 Read-only,for_each,Nikolas Klauser,`D124332 <https://llvm.org/D124332>`_,✅
----------------
Missing link.


================
Comment at: libcxx/include/__algorithm/is_permutation.h:99
+  while (__first1 != __last1 && __first2 != __last2) {
+    if (!std::__invoke(__pred, std::__invoke(__proj1, *__first1), std::__invoke(__proj2, *__first2)))
       break;
----------------
Question: using `__invoke` slightly changes the behavior of the non-ranges version of the algorithm as well, right?


================
Comment at: libcxx/include/__algorithm/is_permutation.h:172
                _ForwardIterator2 __last2) {
-  typedef typename iterator_traits<_ForwardIterator1>::value_type __v1;
-  typedef typename iterator_traits<_ForwardIterator2>::value_type __v2;
+  typedef __iter_value_type<_ForwardIterator1> __v1;
+  typedef __iter_value_type<_ForwardIterator2> __v2;
----------------
Optional: can you use `using` like other similar places that were refactored in this patch?


================
Comment at: libcxx/include/__algorithm/ranges_is_permutation.h:26
+
+_LIBCPP_BEGIN_NAMESPACE_STD
+
----------------
Please add the language version/no incomplete ranges guard.


================
Comment at: libcxx/include/__algorithm/ranges_is_permutation.h:44
+                  _Proj2 __proj2 = {}) const {
+    return std::__is_permutation(__first1, __last1, __first2, __last2, __pred, __proj1, __proj2);
+  }
----------------
Nit: please move the iterators (and add the include).


================
Comment at: libcxx/include/__algorithm/ranges_is_permutation.h:54
+  bool operator()(_Range1&& __range1, _Range2&& __range2, _Pred __pred = {}, _Proj1 __proj1 = {}, _Proj2 __proj2 = {}) const {
+    if constexpr (sized_range<_Range1> && sized_range<_Range2>) {
+      if (ranges::distance(__range1) != ranges::distance(__range2))
----------------
Can we do the same optimization in the iterator version (by checking `sized_sentinel_for`)?


================
Comment at: libcxx/include/__algorithm/ranges_is_permutation.h:59
+
+    return std::__is_permutation(ranges::begin(__range1), ranges::end(__range1),
+                                 ranges::begin(__range2), ranges::end(__range2),
----------------
Should this go to the `else` branch of the `if constexpr`?


================
Comment at: libcxx/include/algorithm:1121
 #include <__algorithm/ranges_fill.h>
 #include <__algorithm/ranges_fill_n.h>
 #include <__algorithm/ranges_find.h>
----------------
Missing synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.is_permutation/ranges.is_permutation.pass.cpp:8
+//===----------------------------------------------------------------------===//
+
+#include <algorithm>
----------------
Missing synopsis.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.is_permutation/ranges.is_permutation.pass.cpp:87
+
+  // the first range is empty
+  test<Iter1, Sent1, Iter2, Sent2, 0, 4>({.input1 = {}, .input2 = {4, 3, 2, 1}, .expected = false});
----------------
Please also check with a single-element range.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.is_permutation/ranges.is_permutation.pass.cpp:95
+  test<Iter1, Sent1, Iter2, Sent2, 0, 0>({.input1 = {}, .input2 = {}, .expected = true});
+
+}
----------------
Nit: there's an unnecessary blank line.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.is_permutation/ranges.is_permutation.pass.cpp:109
+
+constexpr bool test() {
+  test_iterators1<forward_iterator<int*>, sentinel_wrapper<forward_iterator<int*>>>();
----------------
Please also add test cases that use a non-default projection or a non-default predicate.


================
Comment at: libcxx/test/std/algorithms/alg.nonmodifying/alg.is_permutation/ranges.is_permutation.pass.cpp:190
+
+  { // check that the complexity requirements are met with identical ranges
+    {
----------------
Should be `ranges of different sizes` or similar. Also, is there a reason that these test cases aren't part of the `test` function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127194