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

[Konstantin Varlamov via Phabricator via libcxx-commits]

var-const added inline comments.


================
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);
----------------
Quuxplusone wrote:
> Grammar nit: `AssignableOnlyFromInt`
Done.

(I'm not a native speaker, but I thought both are fine? At least, I've definitely seen "only" used at the end of a sentence quite a bit, e.g. https://en.wikipedia.org/wiki/For_Your_Eyes_Only)


================
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 {
----------------
Quuxplusone wrote:
> 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*)>);
> ```
Done. I still think it's good to check whether the comparator defines a strict weak order because it makes the intention of the test clearer (and I created aliases for the function types).


================
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*>())))>);
+
----------------
Quuxplusone wrote:
> 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.
Replaced with the tests you provided, thanks (also added some type aliases).

`struct Projection` is intended to be used as a comparator, right? It does work in practice (also when `operator()` is not const). It's a good question whether it //should// work, though. From a glance, I feel like we shouldn't demand the invocation operator to be const (we take it by value and don't have to make our copy const, after all). I think the pre-ranges algorithms allow functors to take their arguments by a non-const reference, but if the functor modifies the values, it's undefined behavior, and I suppose an argument could be made that ranges algorithms should also support this for ease of conversion. As for rvalue arguments, I'm not sure. I don't think we need to solve this in this patch, though.


================
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>
----------------
Quuxplusone wrote:
> 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
Done. This was formatted manually, clang-format actually merges these into a single line.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119489