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

[Hui via Phabricator via libcxx-commits]

huixie90 marked 2 inline comments as done.
huixie90 added inline comments.


================
Comment at: libcxx/test/std/algorithms/alg.sorting/alg.merge/ranges_merge.pass.cpp:103-104
+
+template <class It, class St = sentinel_wrapper<It>>
+using Range = UncheckedRange<It, St>;
+
----------------
philnik wrote:
> Is there a reason you add an alias here?
I took your suggestion and used `UncheckedRange`. But I can't figure out what `UncheckedRange` means. I was confused in the test points below if I write many `UncheckedRange` without understanding what are "unchekced" in those test points' context. All they need is just a range so I made an alias


================
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)...); };
+
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > 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.
> > regarding "default them to something sensible", I disagree. I actually had a read on the "range_move" test and I saw this 
> > ```
> > static_assert(HasMoveIt<int*>);
> > ```
> > I was very confused about what it means. I thought it means it is ok to do `std::ranges::move(int*)`, which doesn't make sense to me.
> > 
> > I think for all the arguments that can have sensible defaults, the `std::ranges::merge` already have the defaults, e.g. comparator, projection. for all other arguments, two input iterators, two sentinels, one output iterator, they don't have sensible defaults otherwise the stl would give them. creating some defaults which changes the order of the arguments than the std::ranges::merge's arguments would make the tests even harder to read. 
> > 
> > Part of the problem is that this algorithm takes 5 iterator arguments, which makes reading the code really hard. But it is similar to read the actual caller's code (with the same arguments order). I would rather keep it as it is than creating new confusion.
> > 
> > regarding `HasMerge`, I am happy to change if it is the libcxx's convention. Personally I found `HasMerge` not very suitable for algorithms as it sounds like " type T has a member function `merge` " rather than `the algorithm merge can be called with these types"
> For actually calling the `ranges::merge` I agree that it doesn't make sense to add any other default arguments, but we are talking about the types here, so it makes sense that `class Iter, class Sent = sentinel_wraper<Iter>`. My main problem is that there are 5 arguments, which makes it almost impossible so look at it and see what you are testing. I'd be happy if you add `HasMergeIt_R1`, `HasMergeIt_R2` and `HasMergeIt_Out` or something like that if you think the default arguments are a bit obscure (they might be for two ranges plus an iterator). There you could just have `template <class Iter, class Sent = sentinel_wrapper<Iter>>` and then instantiate the other arguments with default values. IMO the current state is really hard to read.
Yes I agree the problem is that `merge` takes 5 arguments. but I can't come up with a helper concept that can be shorter and clear enough what these arguments really are at the same time. what about for these tests, format them as a table

```
//                        InIter1       Sent1    InIter2    Sent2      OutIter
static_assert( HasMerge<  int*,         int*,      int*,     int*,      int*>);     
static_assert(!HasMerge<  int*,        long*,      int*,     int*,      int*>);     
```


================
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*> >;
+
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > This seems overly complicated. It would be a lot easier to just have a few functions to make the cartesian product.
> > I don't think writing few functions can lead less code (as a caller) as you have to list these types anyway.
> > 
> > I wish I could use boost::hana but I think if we can lift this cartesian_product as a general test utilities it would be really useful. writing several concrete test functions to simulate cartesian_product wouldn't be reusable 
> You can't reuse functions, but It really doesn't lead to a lot of boiler code and is easily comprehensible as it doesn't require heaps of meta-programming. 
that is why I'd suggest to have a cartesian_product utility somewhere else and tested. so all the tests would just used it without writing meta-programming. 


================
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());
----------------
philnik wrote:
> huixie90 wrote:
> > philnik wrote:
> > > Could you make the cartesian product of this? (Just copy-paste it and change the types.)
> > again, "just copy-paste" doesn't sound right to me even it is in the test. I really think we should have tested cartesian_product (for types) utility 
> As I said a few times already, having met-programming makes tests harder to understand. I also don't like code duplication, but having hard-to-read tests is a lot worse.
My point is that you do meta-programming once (in a utility header), and you will made the tests much clearer


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128611