[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/__functional/bind_back.h:1
+// -*- C++ -*-
+//===----------------------------------------------------------------------===//
----------------
High-level question: Why is this so complicated? All it needs to do AFAICT is just take the N arguments passed to it (for N=1 in C++20, but arguably we might see N>1 at some point), keep them in a tuple of `args...`, and then later take a `range auto&& lhs` and call the bound function `__f(lhs, args...)`. This doesn't feel like it ought to involve such a generic name as `__invoke_shuffle`.  Is this overengineered, or what am I missing? 


================
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>>;
----------------
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?


================
Comment at: libcxx/include/__ranges/transform_view.h:422
+  inline constexpr auto transform = __transform_adaptor{};
+} // end namespace views
+
----------------
In `all.h`, we create a nested namespace `views::__all`, place `struct __fn` in there, and then define the niebloid itself in
```
inline namespace __cpo {
  inline constexpr auto all = __all::__fn{};
} // namespace __cpo
```
(all this basically to avoid unintentional ADL when `views::all` is passed as a function argument). If there's no technical reason to diverge here, please stick to that pattern; it'll make it easier to tell what's significant in this PR.


================
Comment at: libcxx/include/module.modulemap:422
       module bind                       { private header "__functional/bind.h" }
+      module bind_back                  { private header "__functional/bind_back.h" }
       module bind_front                 { private header "__functional/bind_front.h" }
----------------
Naming nit: It looks like our style for implementation-detail headers is `module __bind_back` and `__functional/__bind_back.h`. Compare to `__decay_copy.h` below; and I've proliferated the underscored style in my [cmp.alg] patch.
(However, `perfect_forward.h` seems to break the pattern already.)


================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/adaptor.pass.cpp:52
+  return 0;
+}
----------------
I'd like to see an explicit test here that `r | std::views::transform` is not supported. E.g.
```
template<class T>
auto can_be_piped(T&& t) -> decltype("abcd" | static_cast<T&&>(t), std::true_type())
{
    return std::true_type();
}

std::false_type can_be_piped(...) { return std::false_type{}; }

assert( can_be_piped(std::views::all));
assert(!can_be_piped(std::views::transform));
assert( can_be_piped(std::views::transform([](char){return 1;})));
```
We also want a test for the (highly unfortunate) fact that `auto x = std::views::transform(1)` is valid. It's not //pipeable//, but it's //valid// to create such an object as long as you don't try to use it for anything meaningful. (I dunno, it might be IFNDR somehow; but GCC and MSVC accept it.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098