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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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
+
----------------
ldionne wrote:
> Is this `#if` wrong, or is the `#endif` comment wrong?
Fixed. I think the `#if` is correct -- we only guard `sized_sentinel_for` with the language version, so it's okay to use regardless of whether ranges are enabled, and it covers more cases than the more simplistic check in the `#else` branch.


================
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;
----------------
ldionne wrote:
> 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.
I'd like to try it out, but would leave it to a follow-up patch.


================
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>());
----------------
ldionne wrote:
> 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.
The `_ConstTimeDistance == true` overload relies on the ability to delegate to the `_ConstTimeDistance == false` overload (which is easy because it can pass `false_type` directly instead of using the trait). It seems like it would be more difficult to achieve with `enable_if` (unless there's some approach I didn't think of).


================
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),
----------------
ldionne wrote:
> 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.
Done. I found myself getting confused between all the different overloads, but it's a good point that having a single name would be consistent with 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;
+    }
----------------
ldionne wrote:
> Is this still needed here, or do we handle this in `__is_permutation`?
Unfortunately, I think it's still necessary. It's possible for the range to be a `sized_range` without the sentinel satisfying `sized_sentinel_for` -- e.g. the range type could define `size()` member function but the sentinel is just a forward iterator that doesn't support subtracting (on which `sized_sentinel_for` relies).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127194