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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

LGTM mod comments, but I think someone else should also take a look.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:31
+using Output = cpp17_output_iterator<int*>;
+static_assert( std::weakly_incrementable<Output>);
+
----------------
Huh, I didn't notice until now that `mergeable` does not actually require that `O` be an `output_iterator` — i.e. `mergeable<A,B,O>` doesn't subsume `output_iterator<O, iter_value_t<I>>`.
Perhaps you should define your own `struct Output` here that satisfies only the very minimal requirements and is //not// actually an output iterator; is that physically possible?


================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:81-82
+// No indirect strict weak order between I1 and I2 (bad projection).
+using ProjValue = int(*)(int);
+using ProjPtr = int*(*)(int);
+static_assert( std::mergeable<Input, Input, Output, GoodComp, std::identity, std::identity>);
----------------
How about naming these `ToInt` and `ToPtr`?
(Or of course eliminating //all// the typedefs. Implicit in my suggestions here is that I find simple types like `int*` much easier to parse at a glance, than typedefs like `Input` and `Output` where it's not even clear that they're iterators, let alone that their value type is `int`. So anywhere that the iterator type is "uninteresting," I'd much prefer to see a boring type like `int*` instead of a typedef. This even extends to the function types: I find `bool(*)(int, int)` easier to parse than `GoodComp`.)


================
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*>())))>);
+
----------------
var-const wrote:
> 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.
> `struct Projection` is intended to be used as a comparator, right?

Oops, yeah. I think I started my thought thinking "projection," but then muscle memory turned it into a comparator halfway through. I think the interesting case I was trying to get at was
```
struct Projection {
    int operator(int&) const;
    int operator(int&&) const = delete;
};
```
i.e. making sure that the projection was called as `proj(*first)` with `*first` as a mutable lvalue.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119489