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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__ranges/range_adaptor.h:45-60
+template <ranges::viewable_range _View, class _Closure>
+    requires _RangeAdaptorClosure<decay_t<_Closure>>
+_LIBCPP_HIDE_FROM_ABI
+constexpr auto operator|(_View&& __view, _Closure&& __closure)
+noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view))))
+-> decltype(      _VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view)))
+{ return          _VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view)); }
----------------
cjdb wrote:
> ldionne wrote:
> > cjdb wrote:
> > > cjdb wrote:
> > > > cjdb wrote:
> > > > > 
> > > > I'd //really// like for us to make these hidden friends. It's definitely doable for `range | closure`, and I //think// it's doable for `closure | closure` too.
> > > What am I saying? If it's possible for `range | closure`, of course it's possible for `closure | closure` :facepalm:
> > This is what we'd end up with:
> > 
> > ```
> > template <class _Closure>
> > struct __range_adaptor_closure {
> >     template <ranges::viewable_range _View>
> >     _LIBCPP_HIDE_FROM_ABI
> >     friend constexpr auto operator|(_View&& __view, _Closure&& __closure)
> >     noexcept(noexcept(_VSTD::invoke(_VSTD::move(__closure), _VSTD::forward<_View>(__view))))
> >     -> decltype(      _VSTD::invoke(_VSTD::move(__closure), _VSTD::forward<_View>(__view)))
> >     { return          _VSTD::invoke(_VSTD::move(__closure), _VSTD::forward<_View>(__view)); }
> > 
> >     template <ranges::viewable_range _View>
> >     _LIBCPP_HIDE_FROM_ABI
> >     friend constexpr auto operator|(_View&& __view, _Closure const& __closure)
> >     noexcept(noexcept(_VSTD::invoke(__closure, _VSTD::forward<_View>(__view))))
> >     -> decltype(      _VSTD::invoke(__closure, _VSTD::forward<_View>(__view)))
> >     { return          _VSTD::invoke(__closure, _VSTD::forward<_View>(__view)); }
> > 
> >     template <_RangeAdaptorClosure _OtherClosure>
> >     _LIBCPP_HIDE_FROM_ABI
> >     constexpr auto operator|(_Closure&& __c1, _OtherClosure&& __c2)
> >     noexcept(noexcept(__range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), _VSTD::move(__c1)))))
> >     -> decltype(      __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), _VSTD::move(__c1))))
> >     { return          __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), _VSTD::move(__c1))); }
> > 
> >     template <_RangeAdaptorClosure _OtherClosure>
> >     _LIBCPP_HIDE_FROM_ABI
> >     constexpr auto operator|(_Closure const& __c1, _OtherClosure&& __c2)
> >     noexcept(noexcept(__range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), __c1))))
> >     -> decltype(      __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), __c1)))
> >     { return          __range_adaptor_closure_t(_VSTD::__compose(_VSTD::forward<_OtherClosure>(__c2), __c1)); }
> > };
> > ```
> > 
> > A few remarks:
> > 
> > - For `range | closure`, it's not clear to me that we gain much since we need two overloads. And perhaps we'd actually need more to handle `_Closure&` and `_Closure const&&`?
> > - For `closure | closure`, that simply doesn't work because that will cause an ambiguity. If we have two closures `Closure1 c1` and `Closure2 c2`, `c1 | c2` could use either the `operator|(Closure1, auto)` injected by `__range_adaptor_closure<Closure1>`, or the `operator|(Closure2, auto)` injected by `__range_adaptor_closure<Closure2>`. I don't see an easy way to solve that problem.
> > 
> > For those reasons, I'd be tempted to leave both as-is.
> [[ https://godbolt.org/z/az96scex5 | Here's a minimised version ]] of what I have in mind. I don't think there's a lot you'll need to do to integrate it: just decorations and seeing if your existing concepts have a mapping (I couldn't see it: from what I could tell, we need both yours and mine).
I was going to say this:

----------------------------------

I don't see the benefit of using hidden friends in that way. If we have N specializations of `__range_adaptor_closure`, we end up injecting N function overloads like:

```
template<ranges::viewable_range _View, class _C2>
    requires std::same_as<std::remove_cvref_t<_C2>, __range_adaptor_closure<F1>>
constexpr auto operator|(_View&&, _C2&&);

template<ranges::viewable_range _View, class _C2>
    requires std::same_as<std::remove_cvref_t<_C2>, __range_adaptor_closure<F2>>
constexpr auto operator|(_View&&, _C2&&);

...

template<ranges::viewable_range _View, class _C2>
    requires std::same_as<std::remove_cvref_t<_C2>, __range_adaptor_closure<FN>>
constexpr auto operator|(_View&&, _C2&&);
```

It seems strictly simpler (and more compile-time efficient) to inject a single function template like:

```
template <ranges::viewable_range _View, _RangeAdaptorClosure _Closure>
constexpr auto operator|(_View&&, _Closure&&);
```

That way, overload resolution has a single additional candidate to consider instead of N additional candidates. Also, the error messages will show only one candidate, as opposed to the N candidates with the friend injection approach.

----------------------------------

But then I reviewed how hidden friends worked again and learned that a hidden friend was only considered by overload resolution if it was in an associated namespace of one of the arguments. In other words, I thought hidden friends were injected in the enclosing namespace and from then on always considered for overload resolution, but that's not the case. TIL!



================
Comment at: libcxx/include/__ranges/range_adaptor.h:45-49
+template <ranges::viewable_range _View, _RangeAdaptorClosure _Closure>
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI
+constexpr auto operator|(_View&& __view, _Closure&& __closure)
+noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view))))
+-> decltype(      _VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view)))
----------------
cjdb wrote:
> Unless I'm mistaken, the decltypes are only there for SFINAE friendliness?
We need to use `decltype(auto)` here, but indeed I added the `invocable<...>` requirement. It's not clear to me we need `regular_invocable` semantically - I don't think anything would prevent users from having a stateful function object that isn't equality preserving?


================
Comment at: libcxx/include/__ranges/transform_view.h:414
+    noexcept(noexcept(transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f))))
+    -> decltype(      transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f)))
+    { return          transform_view(_VSTD::forward<_Range>(__range), _VSTD::forward<_Fn>(__f)); }
----------------
cjdb wrote:
> Same dealeo as above: this should be a requirement, not expression-SFINAE. Gonna leave it at three comments, I trust you to find all the rest :-)
For this one, http://eel.is/c++draft/range.adaptors#range.transform.overview-2 says it should be expression equivalent to `transform_view(range, f)`. But you're correct in general for the other ones and I changed a bunch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098