P2387R3 Pipe for user defined range adaptors #2661
Conversation
tests/std/tests/P2387R3_pipe_support_for_user_defined_range_adaptors/test.cpp
Show resolved
Hide resolved
tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp
Outdated
Show resolved
Hide resolved
…range adaptor closures got removed, because it is superfluous. Both will bind to const&
strega-nil-ms
left a comment
strega-nil-ms
left a comment
There was a problem hiding this comment.
I have not yet reviewed the tests, though I have reviewed all the product code.
This looks really fantastic! It looks like it follows the standard pretty directly, and I don't have a lot of concerns with it.
The only major things I see are some weirdnesses with remove_cvref_t, some reorderings, and the ABI concern with _Pipe::_Base and range_adaptor_closure. I recommend making range_adaptor_closure<_Ty> derive from _Pipe::_Base<_Ty> so as to reuse that code, and revert existing uses of _Pipe::_Base back to _Pipe::_Base.
Thank you so much!
strega-nil-ms
left a comment
strega-nil-ms
left a comment
There was a problem hiding this comment.
The tests look great :)
Most notably, `range_adaptor_closure<T>` now derives from `_Pipe::_Base<T>`, and the standard range adaptor closure objects derive directly from `_Pipe::_Base` as they did previously. I also eliminated a ton of repetition by implementing `operator|` for `_Pipe::_Base` as a non-member. (Since the non-member operator is squirreled away in `std::ranges::_Pipe` it is still effectively only found by ADL for things derived from `_Pipe::_Base` despite no longer being a hidden friend.)
tests/std/tests/VSO_0157762_feature_test_macros/test.compile.pass.cpp
Outdated
Show resolved
Hide resolved
|
This is incredible @Ukilele - thanks! I've pushed changes to (1) merge with main and resolve conflicts, (2) address the ABI concerns, and (3) simplify the "is this a range adaptor" concept a bit. |
tests/std/tests/P2387R3_pipe_support_for_user_defined_range_adaptors/env.lst
Show resolved
Hide resolved
|
I'm speculatively mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
| && constructible_from<remove_cvref_t<_Left>, _Left> // | ||
| && constructible_from<remove_cvref_t<_Right>, _Right> // | ||
| _NODISCARD constexpr auto operator|(_Left&& __l, _Right&& __r) noexcept( | ||
| noexcept(_Pipeline{static_cast<_Left&&>(__l), static_cast<_Right&&>(__r)})) { |
There was a problem hiding this comment.
Ugh, this formatting is hideous, but not worth resetting the port. I'll clean these in a followup.
There was a problem hiding this comment.
Thanks. Yeah, I noticed how icky it was - perhaps because [[nodiscard]] is being concealed with the macro.
StephanTLavavej
dismissed
strega-nil-ms’s stale review
Casey addressed Nicole's comments
|
Thanks for making this ranges plumbing available to users! 😹 🎉 🚀 |
9 participants
Fixes #2536