[libcxx-commits] [PATCH] D128611: [libc++] Implement `std::ranges::merge`

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik requested changes to this revision.
philnik added a comment.
This revision now requires changes to proceed.

The implementation looks good, but the tests need some work.



================
Comment at: libcxx/docs/Status/RangesAlgorithms.csv:63
 Write,partial_sort_copy,Not assigned,n/a,Not started
-Merge,merge,Not assigned,n/a,Not started
-Merge,set_difference,Not assigned,n/a,Not started
-Merge,set_intersection,Not assigned,n/a,Not started
-Merge,set_symmetric_difference,Not assigned,n/a,Not started
-Merge,set_union,Not assigned,n/a,Not started
+Merge,merge,Hui Xie,n/a,Under review
+Merge,set_difference,Hui Xie,n/a,Not started
----------------
This should be `✅`, since this done once it's landed.


================
Comment at: libcxx/include/__algorithm/ranges_merge.h:42
+    template <
+        std::input_iterator _InIt1,
+        std::sentinel_for<_InIt1> _Sent1,
----------------
Please don't annotate these unnecessarily.


================
Comment at: libcxx/include/__algorithm/ranges_merge.h:42
+    template <
+        std::input_iterator _InIt1,
+        std::sentinel_for<_InIt1> _Sent1,
----------------
philnik wrote:
> Please don't annotate these unnecessarily.
Please use `_InIter` and `_OutIter`. We discussed this on discord a while back and decided on these.


================
Comment at: libcxx/include/__algorithm/ranges_merge.h:51
+      requires std::mergeable<_InIt1, _InIt2, _OutIt, _Comp, _Proj1, _Proj2>
+    constexpr ranges::merge_result<_InIt1, _InIt2, _OutIt> operator()(
+        _InIt1 __first1,
----------------
You forgot `_LIBCPP_HIDE_FROM_ABI`. Same for the ranges version.


================
Comment at: libcxx/include/__algorithm/ranges_merge.h:60
+        _Proj2 __proj2 = {}) const {
+      for (; !(__first1 == __last1 || __first2 == __last2); ++__result) {
+        if (std::invoke(__comp, std::invoke(__proj2, *__first2), std::invoke(__proj1, *__first1))) {
----------------
I think that's easier to read.


================
Comment at: libcxx/include/__algorithm/ranges_merge.h:96-104
+      return (*this)(
+          ranges::begin(__range1),
+          ranges::end(__range1),
+          ranges::begin(__range2),
+          ranges::end(__range2),
+          std::move(__result),
+          std::move(__comp),
----------------
You have to put the implementation into it's own function to avoid moving the comparator and projections.


================
Comment at: libcxx/include/__algorithm/ranges_merge.h:113
+} // namespace __cpo
+
+} // namespace ranges
----------------
We normally don't put an empty line here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:37-38
+
+template <class... Args>
+concept canMerge = requires(Args&&... args) { std::ranges::merge(std::forward<Args>(args)...); };
+
----------------
Could you list the arguments and default them to something sensible? The current test is very hard to read. Also, please rename this to `HasMerge` to go with the naming style in the other algorhtm tests.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:44-53
+static_assert(canMerge<int*, int*, int*, int*, int*>);
+static_assert(canMerge<int*, sentinel_wrapper<int*>, int*, sentinel_wrapper<int*>, int*>);
+static_assert(canMerge<cpp20_input_iterator<int*>, sentinel_wrapper<cpp20_input_iterator<int*>>, int*, int*, int*>);
+static_assert(canMerge<forward_iterator<int*>, sentinel_wrapper<forward_iterator<int*>>, int*, int*, int*>);
+static_assert(canMerge<int*, int*, bidirectional_iterator<int*>, sentinel_wrapper<bidirectional_iterator<int*>>, int*>);
+static_assert(canMerge<int*, int*, random_access_iterator<int*>, sentinel_wrapper<random_access_iterator<int*>>, int*>);
+static_assert(canMerge<int*, int*, contiguous_iterator<int*>, sentinel_wrapper<contiguous_iterator<int*>>, int*>);
----------------
All the positive tests should be checked by runtime tests and not by `static_assert`s except for a simple one to ensure the concept can return true.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:80-85
+// Negative tests: not enough input arguments
+static_assert(!canMerge<>);
+static_assert(!canMerge<int*>);
+static_assert(!canMerge<int*, int*>);
+static_assert(!canMerge<int*, int*, int*>);
+static_assert(!canMerge<int*, int*, int*, int*>);
----------------
These checks seem not very useful to me. We don't check that for anything else. These is also nothing that could easily trigger these tests, other than adding an overload or adding more default values.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:100
+
+// Negative tests: !std::sentinel_for<S2, I2>
+static_assert(!canMerge<int*, int*, int*, long*, int*>);
----------------
We have `NotSentinelForX` for this.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:105
+
+// Negative tests: !std::weakly_incrementable<O>
+static_assert(!canMerge<int*, int*, int*, int*, std::optional<int>>);
----------------
We have `WeaklyIncrementableNotMovable` for this.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:108-111
+// Negative tests: !std::indirectly_copyable<I1, Out>
+static_assert(!canMerge<int*, int*, int*, int*, const int*>);
+static_assert(!canMerge<Bar*, Bar*, Foo*, Foo*, Foo*, CompareOverloadSet>);
+static_assert(!canMerge<MoveOnly*, MoveOnly*, MoveOnly*, MoveOnly*, MoveOnly*>);
----------------
I don't think we need three tests for the same diagnostic. This is only to ensure that we have constrained the algorithm, not to test the concepts.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:127-131
+template <class It, class St = sentinel_wrapper<It>>
+struct Range {
+  It begin() const;
+  St end() const;
+};
----------------
We have `UncheckedRange` for this.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:134
+// Positive tests: different categories of input ranges and output iterator
+static_assert(canMerge<Range<int*, int*>, Range<int*, int*>, int*>);
+static_assert(canMerge<Range<int*>, Range<int*>, int*>);
----------------
All the comments above also apply here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:207-209
+  std::array i1{0, 1, 5, 6, 9, 10};
+  std::array i2{3, 6, 7, 9, 13, 15, 100};
+  std::array expected{0, 1, 3, 5, 6, 6, 7, 9, 9, 10, 13, 15, 100};
----------------
This should be tested with different inputs.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:219
+  {
+    std::array<int, 13> out{};
+    std::same_as<std::ranges::merge_result<In1, In2, Out>> decltype(auto) result = std::ranges::merge(
----------------
Please don't value-initialized the arrays. That ensures that the `out` array was actually written to properly, since reading from it would error during constant evaluation.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:228
+
+    static_assert(std::same_as<decltype(result.in1), In1>);
+    assert(result.in1 == Sent1{In1{i1.data() + i1.size()}});
----------------
Aren't these already checked by the `same_as` above?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:229
+    static_assert(std::same_as<decltype(result.in1), In1>);
+    assert(result.in1 == Sent1{In1{i1.data() + i1.size()}});
+
----------------
Why don't you use `base()`?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:240
+  {
+    std::array<int, 13> out{};
+    std::ranges::subrange r1{In1{i1.data()}, Sent1{In1{i1.data() + i1.size()}}};
----------------
Again, all the comments from above also apply here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:263-307
+struct TracedCopyMove {
+  int copy_ctor   = 0;
+  int move_ctor   = 0;
+  int copy_assign = 0;
+  int move_assign = 0;
+
+  int data = 0;
----------------
This class seems to be a little over-engineered. 
```
    struct CopyOnce {
      bool copied = false;
      constexpr CopyOnce() = default;
      constexpr CopyOnce(const CopyOnce& other) = delete;
      constexpr CopyOnce& operator=(const CopyOnce& other) {
        assert(!other.copied);
        copied = true;
        return *this;
      }
    };
```
should do the trick.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:318
+struct Data {
+  int data = 0;
+};
----------------
I don't think you are using the default value anywhere.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:324-347
+    using In1s = type_list<
+        cpp20_input_iterator<int*>,
+        forward_iterator<int*>,
+        bidirectional_iterator<int*>,
+        random_access_iterator<int*>,
+        contiguous_iterator<int*> >;
+
----------------
This seems overly complicated. It would be a lot easier to just have a few functions to make the cartesian product.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:352-360
+    std::array r1{3, 6, 7, 9};
+    std::array<int, 7> out;
+    std::same_as<
+        std::ranges::
+            merge_result<std::ranges::iterator_t<std::array<int, 4>&>, std::ranges::dangling, int*>> decltype(auto)
+        result = std::ranges::merge(r1, std::array{2, 3, 4}, out.data());
+    assert(result.in1 == r1.end());
----------------
Could you make the cartesian product of this? (Just copy-paste it and change the types.)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:356
+        std::ranges::
+            merge_result<std::ranges::iterator_t<std::array<int, 4>&>, std::ranges::dangling, int*>> decltype(auto)
+        result = std::ranges::merge(r1, std::array{2, 3, 4}, out.data());
----------------
Please try to avoid using meta-programming to check meta-programming. This should simply be `std::array<int, 4>::iterator`, or (IMO the better option) use `a.data() + a.size()` or a C-style array and make it `int*`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:367
+    std::array r2{TracedCopyMove{1}, TracedCopyMove{3}, TracedCopyMove{8}};
+    using iter = std::ranges::iterator_t<std::array<TracedCopyMove, 3>&>;
+
----------------
Again, please avoid meta-programming. (And we normally use CamelCase for types)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:385
+          std::ranges::merge(r1.begin(), r1.end(), r2.begin(), r2.end(), out.data());
+      verify(result, out);
+    }
----------------
Could you just copy-paste the asserts? That makes it a lot easier to verify at a glance.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:399
+  {
+    std::array r1{IntAndID{0, 0}, IntAndID{0, 1}, IntAndID{0, 2}};
+    std::array r2{IntAndID{1, 0}, IntAndID{1, 1}, IntAndID{1, 2}};
----------------
Could you put the structs locally? `IntAndID` isn't the most helpful name. (When you do that you might as well rename it to `S`, but I don't care much)


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:400
+    std::array r1{IntAndID{0, 0}, IntAndID{0, 1}, IntAndID{0, 2}};
+    std::array r2{IntAndID{1, 0}, IntAndID{1, 1}, IntAndID{1, 2}};
+
----------------
I also like CTAD, but here I think it would be easier to use `std::array<IntAndID, 3>`.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:403
+    const auto verify = [&](const std::array<IntAndID, 6>& out) {
+      assert(std::ranges::equal(out | std::views::transform(&IntAndID::data), std::array{0, 0, 0, 1, 1, 1}));
+
----------------
Couldn't you use the projection?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:409
+
+    // range overload
+    {
----------------
Could you stay consistent in what order you put these?


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:413
+      std::ranges::merge(r1, r2, out.data());
+      verify(out);
+    }
----------------
Again, please just copy-paste the assertions.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:424
+
+  // Equal elements in R1 should be merged before equal elements in R2
+  {
----------------
This seems to be a copy-paste of the last test.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:452
+  {
+    std::array r1{Data{4}, Data{8}, Data{12}};
+    std::array r2{Data{5}, Data{9}};
----------------
Again, please just put the struct right here.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:517
+  }
+
+  return true;
----------------
You are missing test cases for
- using `std::invoke`
- complexity
- that the return type of the comparator is convertible to bool
- value edge-cases (like copying zero-size ranges)


================
Comment at: libcxx/test/std/library/description/conventions/customization.point.object/niebloid.compile.pass.cpp:101
 static_assert(test(std::ranges::max_element, a));
-//static_assert(test(std::ranges::merge, a, a, a));
+static_assert(test(std::ranges::merge, a, a, a));
 static_assert(test(std::ranges::min, a));
----------------
There should also be tests in `ranges_robust_against_copying_comparators.pass.cpp` and `ranges_robust_against_copying_projections.pass.cpp`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128611