[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:31
+using Output = cpp17_output_iterator<int*>;
+static_assert( std::weakly_incrementable<Output>);
+
----------------
Quuxplusone wrote:
> 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?
Yes, it's possible (the way I found is this: `weakly_incrementable` only requires `it++` to be defined, while `output_iterator` requires it to return something dereferenceable, thus making it return `void` does the trick). Added a test case for that.


================
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>);
----------------
Quuxplusone wrote:
> 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`.)
Thanks, I like these names more.

(I think this depends a lot on personal preference. I'm the other way round -- I find a wall of pointer types with small variations throughout to be quite unreadable, and I think "parsing" natural language is easier than special symbols. Also, using typedefs makes it easier to change the types throughout the file)


================
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:
> 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.
`Projection` also works with the current implementation (also with libstdc++ and MSVC implementations).

I think this should work -- `mergeable` requires the underlying type to be copyable, not just movable between the iterators. If the implementation deals with a copy of the value, restricting the projection to e.g. only const references seems to go against that contract.

Do you want to add `Projection` as a test case (i.e., do we want to explicitly commit to supporting this)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119489