brevzin / cpp_proposals

My WG21 proposals
35 stars 22 forks source link

Feedback on 'P2387R3: Pipe support for user-defined range adaptors' #42

Closed mtezych closed 1 month ago

mtezych commented 1 year ago

Hello Barry, I've recently implemented in C++20 a custom range adaptor cxx::chunk_evenly():

While doing so, I've realized that, in order to have the ability to use the pipe operator, I need to implement the std::ranges::range_adaptor_closure class template from P2387R3:

After getting myself familiar with P2387R3, I want to say that, I am strongly in favor of it and glad that it was accepted into the C++23 draft. Finally all C++ developers will have the ability to implement custom range adaptors, which will be composable with standard range adaptors!

However, even though the cxx::ranges::range_adaptor_closure_interface class template does not support composing, via the pipe operator, custom range adaptor closures with standard range adaptor closures, and therefore, when creating a pipeline, the cxx::chunk_evenly() range adaptor is not composable with standard range adaptors:

I hope this feedback is valuable and it is not tool late to at least rename the std::ranges::range_adaptor_closure to std::ranges::range_adaptor_closure_interface before C++23 will approved and released.

Thank you, Mateusz Zych

frederick-vs-ja commented 1 year ago

viewable_range seems pointless at least after P2387. It is only used in the standard library via views::all_t, but given views::all is well-constrained (due to the fix in LWG3724), perhaps we should just rely on the native constraints on views::all.

The remain use of viewable_range in [range.adaptor.object] looks like a defect to me.

frederick-vs-ja commented 1 year ago

the cxx::chunk_evenly() range adaptor is not composable with standard range adaptors:

It seems that the implementation of range_adaptor_closure must be coupled with the implementation details of standard range adaptors to support composing. This doesn't seem an issue to me given range_adaptor_closure is std::ranges::range_adaptor_closure.