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

Louis Dionne via Phabricator via libcxx-commits libcxx-commits at lists.llvm.org
Tue Aug 2 14:11:33 PDT 2022


ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.


================
Comment at: libcxx/include/__algorithm/is_permutation.h:34
 
-  //  __first1 != __last1 && *__first1 != *__first2
-  typedef typename iterator_traits<_ForwardIterator1>::difference_type _D1;
-  _D1 __l1 = _VSTD::distance(__first1, __last1);
-  if (__l1 == _D1(1))
-    return false;
-  _ForwardIterator2 __last2 = _VSTD::next(__first2, __l1);
-  // For each element in [f1, l1) see if there are the same number of
-  //    equal elements in [f2, l2)
-  for (_ForwardIterator1 __i = __first1; __i != __last1; ++__i) {
+#if _LIBCPP_STD_VER > 17
+
----------------
Is this `#if` wrong, or is the `#endif` comment wrong?


================
Comment at: libcxx/include/__algorithm/is_permutation.h:59
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 bool
+__equal_elements(_Iter1 __first1, _Sent1 __last1, _Iter2 __first2, _Sent2 __last2,
+                 _Pred&& __pred, _Proj1&& __proj1, _Proj2&& __proj2) {
----------------
I would suggest naming this `__is_permutation_impl` or something like that. I think it expresses most clearly what the function actually does.

Otherwise, I was trying to understand how it was different from `std::equal_range` (which has very little in common).


================
Comment at: libcxx/include/__algorithm/is_permutation.h:100
+                 _BinaryPredicate&& __pred) {
+  //  shorten sequences as much as possible by lopping of any equal prefix
+  for (; __first1 != __last1; ++__first1, (void)++__first2) {
----------------
Here and elsewhere.


================
Comment at: libcxx/include/__algorithm/is_permutation.h:101-107
+  for (; __first1 != __last1; ++__first1, (void)++__first2) {
+    if (!__pred(*__first1, *__first2))
+      break;
+  }
+
+  if (__first1 == __last1)
+    return true;
----------------
FWIW I think this is a reimplementation of `std::mismatch`.

I don't mind whether we refactor this now or after (or not at all if it turns out not to be a good idea). But we should at least look into it.


================
Comment at: libcxx/include/__algorithm/is_permutation.h:175-181
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 bool
+__is_permutation4(_Iter1 __first1, _Sent1 __last1, _Iter2 __first2, _Sent2 __last2,
+                 _Pred&& __pred, _Proj1&& __proj1, _Proj2&& __proj2) {
+  return std::__is_permutation4<_AlgPolicy>(
+      std::move(__first1), std::move(__last1), std::move(__first2), std::move(__last2),
+      __pred, __proj1, __proj2,
+      _ConstTimeDistance<_Iter1, _Sent1, _Iter2, _Sent2>());
----------------
Instead, why not have just two versions of `__is_permutation4` and use `enable_if_t`? I don't think you need this function at all.


================
Comment at: libcxx/include/__algorithm/ranges_is_permutation.h:41
+                                  _Pred& __pred, _Proj1& __proj1, _Proj2& __proj2) {
+    return std::__is_permutation4<_RangeAlgPolicy>(
+        std::move(__first1), std::move(__last1), std::move(__first2), std::move(__last2),
----------------
I would suggest looking into naming `__is_permutation4` and `__is_permutation3` just `__is_permutation`. That way, they can be called as `std::__is_permutation<Policy>`, which is nicely consistent with what we do for other algorithms.


================
Comment at: libcxx/include/__algorithm/ranges_is_permutation.h:68-71
+    if constexpr (sized_range<_Range1> && sized_range<_Range2>) {
+      if (ranges::distance(__range1) != ranges::distance(__range2))
+        return false;
+    }
----------------
Is this still needed here, or do we handle this in `__is_permutation`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127194



More information about the libcxx-commits mailing list