[libcxx-commits] [PATCH] D119489: [libc++][ranges] Implement `std::mergeable`.

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

Looks pretty reasonable to me, mod comments.



================
Comment at: libcxx/include/__iterator/mergeable.h:16
+#include <__functional/ranges_operations.h>
+#include <__iterator/concepts.h>
+
----------------
Here you need `__iterator/projected.h` and maybe some others; but the Modular CI build should tell you about it.


================
Comment at: libcxx/include/__iterator/mergeable.h:26-27
+
+template<class _Input1, class _Input2, class _Output, class _Comp = ranges::less, class _Proj1 = identity,
+    class _Proj2 = identity>
+concept mergeable =
----------------
Please put the linebreak before `class _Comp`. (At least not between `Proj1` and `Proj2`.)


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:18
+
+#include <compare>
+#include <functional>
----------------
Why is `<compare>` needed?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:49
+struct NotWeaklyIncrementable {
+  int& operator*();
+};
----------------
`int& operator*() const;` ?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:58
+// I1 or I2 is not indirectly copyable into O.
+struct AssignableFromIntOnly {
+  AssignableFromIntOnly& operator=(int);
----------------
Grammar nit: `AssignableOnlyFromInt`


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:73
+
+// No indirect strict weak order between I1 and I2 (bad comparison functor).
+struct BadComp {
----------------
Because `invocable<BadComp, int&, int&>` is false, right? I think it would be cleaner to replace lines 73–79 with just
```
static_assert( std::mergeable<InputIterator, InputIterator, OutputIterator, bool(*)(int, int)>);
static_assert(!std::mergeable<InputIterator, InputIterator, OutputIterator, bool(*)(int*, int*)>);
```


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:89
+static_assert(!std::indirect_strict_weak_order<Comp, InputIterator,
+    decltype(BadProjection()(InputIterator(std::declval<int*>())))>);
+
----------------
I might not be parsing this correctly, but isn't `decltype(BadProjection()(InputIterator(std::declval<int*>())))` simply `Empty`?

Here I think it would be cleaner to replace lines 81–92 with something like
```
static_assert( std::mergeable<const int*, const int*, int*, bool(*)(int, int), std::identity, std::identity>);
static_assert( std::mergeable<const int*, const int*, int*, bool(*)(int, int), int(*)(int), int(*)(int)>);
static_assert(!std::mergeable<const int*, const int*, int*, bool(*)(int, int), int*(*)(int), int(*)(int)>);
static_assert(!std::mergeable<const int*, const int*, int*, bool(*)(int, int), int(*)(int), int*(*)(int)>);
static_assert(!std::mergeable<const int*, const int*, int*, bool(*)(int*, int), int*(*)(int), int(*)(int)>);
static_assert(!std::mergeable<const int*, const int*, int*, bool(*)(int, int*), int(*)(int), int*(*)(int)>);
```
This doesn't cover anything about value categories / ref-qualification, though, so maybe think about whether we expect something like
```
struct Projection {
    bool operator(int&, int&) const;
    bool operator(int&&, int&&) const = delete;
};
```
to work, not-work, or be IFNDR; and whether removing the `const` from `operator()` would break anything.


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.subsumption.compile.pass.cpp:20-27
+template <class I1, class I2, class O>
+void test_subsumption() requires std::input_iterator<I1> && std::input_iterator<I2>;
+template <class I1, class I2, class O>
+void test_subsumption() requires std::weakly_incrementable<O>;
+template <class I1, class I2, class O> void test_subsumption()
+    requires std::indirectly_copyable<I1, O> && std::indirectly_copyable<I2, O>;
+template <class I1, class I2, class O>
----------------
Please add blank lines between these definitions to keep them from all running together visually (since they don't fit on a single line each).

I see clang-format thinks line 25 is a "continuation line" rather than a "separate line"? :P


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119489