[libcxx-commits] [PATCH] D128983: [libcxx][ranges] implement `std::ranges::set_difference`

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const requested changes to this revision.
var-const added a comment.
This revision now requires changes to proceed.

Looks pretty good overall, the only substantial comment is about avoid observable changes in behavior in the classic `set_difference` algorithm. Thanks for working on this!



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:64
 Merge,merge,Hui Xie,n/a,✅
-Merge,set_difference,Hui Xie,n/a,Not started
+Merge,set_difference,Hui Xie,n/a,✅
 Merge,set_intersection,Hui Xie,n/a,Not started
----------------
Please add a link to this patch.


================
Comment at: libcxx/include/__algorithm/ranges_set_difference.h:46
+      input_iterator _InIter2,
+      std::sentinel_for<_InIter2> _Sent2,
+      weakly_incrementable _OutIter,
----------------
Nit: unnecessary `std::`.


================
Comment at: libcxx/include/__algorithm/ranges_set_difference.h:50
+      class _Proj1 = identity,
+      class _Proj2 = identity >
+    requires mergeable<_InIter1, _InIter2, _OutIter, _Comp, _Proj1, _Proj2>
----------------
Nit: unnecessary (I think) space. Is this done by `clang-format`? If yes, it's not worth changing.


================
Comment at: libcxx/include/__algorithm/set_difference.h:38
+    class _Proj2>
+_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_AFTER_CXX17 pair<typename decay<_InIter1>::type, typename decay<_OutIter>::type>
+__set_difference(
----------------
Does using `decay` here lead to any observable difference?


================
Comment at: libcxx/include/__algorithm/set_difference.h:53
+      ++__result;
+    } else if (std::__invoke(__comp, std::__invoke(__proj2, *__first2), std::__invoke(__proj1, *__first1))) {
+      ++__first2;
----------------
Wouldn't using `invoke` here create an observable change in behavior for the classic algorithm? It seems like it would cause e.g. passing a pointer to member to work with the classic algorithm which wasn't the case before.

Instead, I would strongly recommend the following trick: in the range algorithm, create a lambda that wraps the comparator and the projection functors and pass it as the comparator. It also avoids the need to pass the projections directly. See e.g. `ranges_sort.h`:
```
    auto&& __projected_comp = ranges::__make_projected_comp(__comp, __proj);
    std::__sort_impl(std::move(__first), __last_iter, __projected_comp);
```


================
Comment at: libcxx/include/__algorithm/set_difference.h:78
 template <class _InputIterator1, class _InputIterator2, class _OutputIterator>
-inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17
-_OutputIterator
-set_difference(_InputIterator1 __first1, _InputIterator1 __last1,
-               _InputIterator2 __first2, _InputIterator2 __last2, _OutputIterator __result)
-{
-    return _VSTD::set_difference(__first1, __last1, __first2, __last2, __result,
-                                __less<typename iterator_traits<_InputIterator1>::value_type,
-                                       typename iterator_traits<_InputIterator2>::value_type>());
+inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_AFTER_CXX17 _OutputIterator set_difference(
+    _InputIterator1 __first1,
----------------
Optional: you can also `s/_LIBCPP_INLINE_VISIBILITY/_LIBCPP_HIDE_FROM_ABI/` since you're changing this line anyway. `_LIBCPP_HIDE_FROM_ABI` is the newer name for the same macro (we use it in new code and IMO it's good to update the old code to use the new name when opportunity arises).


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/ranges_set_difference.pass.cpp:96
+
+// !mergeable<iterator_t<R1>, iterator_t<R2>, O, Comp, Proj1, Proj2>
+static_assert(!HasSetDifferenceRange< UncheckedRange<MoveOnly*>, UncheckedRange<MoveOnly*>, MoveOnly*>);
----------------
Ultranit: qualify `std::` for consistency?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/ranges_set_difference.pass.cpp:149
+
+  // range 2 shorter than range 1
+  {
----------------
Can you also check the case when the lengths are the same (but the elements are different)?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/ranges_set_difference.pass.cpp:205
+
+  // check that ranges::dangling is returned for non-borrowed_range and iterator_t is returned for borrowed_range
+  {
----------------
Nit: the test case doesn't seem to check `borrowed_range`, so remove that part of the comment?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/ranges_set_difference.pass.cpp:234
+template <class InIter2, class OutIter>
+constexpr void withAllInIter1Iterators() {
+  testImpl<cpp20_input_iterator<int*>, InIter2, OutIter>();
----------------
Nit: probably needs renaming for consistency with the `merge` test.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/ranges_set_difference.pass.cpp:244
+constexpr void withAllInIter1InIter2Iterators() {
+  withAllInIter1Iterators<cpp20_input_iterator<int*>, OutIter>();
+  withAllInIter1Iterators<forward_iterator<int*>, OutIter>();
----------------
Question: does `cpp17_input_iterator` not work? If yes, can you add a short comment indicating why it doesn't work, to make it clear it wasn't forgotten but deliberately omitted?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.set.operations/set.difference/ranges_set_difference.pass.cpp:261
+  // check that every element is copied exactly once
+  struct TracedCopy {
+    int copy_assign = 0;
----------------
Since this class is already used here and in `merge` test, and will likely be used more in further tests, it should probably be moved into a shared header with helpers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128983