[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: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)); }
----------------
ldionne wrote:
> 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!
> 
> 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

Yep — that's the "hidden" part of "hidden friend idiom". :)  The names are "hidden" from ordinary lookup, and findable only via ADL.  This generally has exactly the effect you thought was going to happen the other way around: hidden friends //shrink// the overload set and thus produce //shorter// error messages, whereas "un-hiding" things tends to //grow// the overload set and produce //longer// candidate lists in error messages.
https://quuxplusone.github.io/blog/2020/12/09/barton-nackman-in-practice/  is highly relevant. (So is a lot of the stuff in https://quuxplusone.github.io/blog/tags/#argument-dependent-lookup generally.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098