[libcxx-commits] [PATCH] D107098: [libc++] Implement the underlying mechanism for range adaptors

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

Quuxplusone added inline comments.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:36
+template <class _Tp>
+concept _RangeAdaptorClosure = derived_from<decay_t<_Tp>, __range_adaptor_closure<decay_t<_Tp>>>;
+
----------------
`remove_cvref_t`, not `decay_t`, surely? (Although if `decay_t` is suspected to be faster because compiler builtins, I'll acquiesce instantly.)


================
Comment at: libcxx/include/__ranges/range_adaptor.h:36
+template <class _Tp>
+concept _RangeAdaptorClosure = derived_from<_Tp, __range_adaptor_closure<_Tp>>;
+
----------------
cjdb wrote:
> I think `__pipeable` is a better name in this case.
+1 for at least something snake-case. (I wouldn't object to `__pipeable`, although it doesn't capture what's going on very well IMHO.) Basically, I don't like the look of the template parameter `_RangeAdaptorClosure _Closure` on line 45: concepts should have snake-case names. `__range_adaptor_closure _Closure` would be better, but you've already gobbled up that name for the class template itself.
Alternatively, abandon the terse syntax (abandon the need for a concept representing the class template) and just write
```
template <ranges::viewable_range _View, class _Closure>
    requires __is_range_adaptor_closure<_Closure>
```
Alternatively alternatively, is @cjdb right that these could just be hidden friends of the `_Closure` class? I could imagine that leading to surprising ambiguities when LHS and RHS are both closure classes, so it might need some fiddling-with; but hidden friends would certainly be nice to have.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:29-33
+template <class _Tp>
+struct __range_adaptor_closure { };
+
+template <class _Tp>
+concept __is_range_adaptor_closure = derived_from<_Tp, __range_adaptor_closure<_Tp>>;
----------------
ldionne wrote:
> Quuxplusone wrote:
> > I think it's good that you picked `all` and `transform` for this PR; that's one "nullary" adaptor and one "unary" adaptor. I'm still confused how you're managing to get away with just one `__range_adaptor_closure` template; it intuitively feels like you should just have one `__nullary_range_adaptor` for things invoked as `rng | x` and one `__unary_range_adaptor` for things invoked as `rng | x(a)`. (These are the only two types of adaptors AFAIK.)
> > How do you avoid making `rng | views::all()` well-formed?
> > ...
> > Ah, I'm slowly seeing it. Your `__range_adaptor_closure` is something that can be piped into; so `views::all` IS-A `__range_adaptor_closure`, and `views::transform` has an `operator()` that //returns// a `__range_adaptor_closure`. But then why does `views::transform` itself inherit from `__range_adaptor_closure`? Does that make `rng | views::transform` (with no args) compile when it shouldn't? Why is that inheritance there?
> > I think it's good that you picked all and transform for this PR; that's one "nullary" adaptor and one "unary" adaptor.
> 
> Yes, that was the goal.
> 
> > it intuitively feels like you should just have one `__nullary_range_adaptor` for things invoked as `rng | x` and one `__unary_range_adaptor` for things invoked as `rng | x(a)`. (These are the only two types of adaptors AFAIK.)
> 
> With http://wg21.link/p2387, it is conceivable that we'd have adaptors that take more than one argument, so that's why I support those more generically. You're right, supporting only nullary or unary adaptors would have been easier (basically it doesn't require implementing a general `bind_back` mechanism).
> 
> > How do you avoid making `rng | views::all()` well-formed?
> 
> `views::all()` is not well-formed itself. You *have* to provide at least one argument if you're going to call `views::all`.
> 
> > But then why does `views::transform` itself inherit from `__range_adaptor_closure`?
> 
> Hmm, you're right, it shouldn't. I fixed it and added a test.
Okay, I get it now (I think). Inheriting from `__range_adaptor_closure` gets you the pipe operator, but `__all::__fn` and `__transform::__fn` provide their own `operator()` overloads; `__range_adaptor_closure` doesn't say anything about `operator()`, just `operator|`.

I'd like to see a test for
```
    static_assert(!std::is_invocable_v<decltype(std::views::all)>);  // invocable but only with one arg
    static_assert(!std::is_invocable_v<decltype(std::views::transform)>);  // invocable but only with one or two args
```
although that might be considered a bit out of scope for this patch.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp:42
+
+  // Test `v | views::transform(f)`
+  {
----------------
Line 34 tests `partial(v)` but not `transform(f)(v)`.
Line 44 tests `v | transform(f)` but not `v | partial`.
I suggest testing all four permutations.


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/types.h:132-142
+struct Twice {
+  constexpr int operator()(int x) { return x + x; }
+};
+
 struct Increment {
   constexpr int operator()(int x) { return x + 1; }
 };
----------------
Is `Twice` non-const-callable on purpose?
Tangent: Since callables should generally be const-callable (especially stateless callables), I'd actually prefer to see `Increment` renamed to something like `PlusOneMutable` and `IncrementConst` renamed to just `PlusOne`. Give the weird name to the weird callable.
Also, if you rename to `PlusOne`, then `Twice` should become `TimesTwo` to match it (and return `x * 2`, too).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098