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

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

Quuxplusone added reviewers: ldionne, Mordante, philnik.
Quuxplusone added a comment.

Continues to LGTM; continue to want one other person to rubber-stamp it at least.



================
Comment at: libcxx/test/std/iterators/iterator.requirements/alg.req.mergeable/mergeable.compile.pass.cpp:97-98
+  WeaklyIncrementable& operator++();
+  // `output_iterator` requires `i++` to return an iterator, while `weakly_incrementable` only requires `i++`  to be
+  // defined.
+  void operator++(int);
----------------
Drive-by grammar nit ;) but also, mainly, adjusted the line-break point.


================
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:
> > 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)?
> Do you want to add `Projection` as a test case (i.e., do we want to explicitly commit to supporting this)?

Yes, my reading of the standard is that we //have// to support this. (In fact, we have to support `Projection` even when its `operator()` is not const-qualified!) Now, probably when we get to the actual `ranges::merge` //algorithm// we'll find it's not so clear-cut, at least in theory; but for `concept mergeable`, the chain of logic/code seems pretty unassailable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119489