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

Thu Aug 26 10:03:53 PDT 2021

cjdb added a comment.

One change left, then I think we're ready to rock! I'm going to conditionally LGTM it on my proposed change, but please ping me on Discord if you want to discuss my proposed change (we can move back here for the actual discussion, but Discord ping > email ping in terms of responsiveness due to Phab latency).

================
Comment at: libcxx/include/__ranges/range_adaptor.h:60-61
+        requires same_as<_Tp, remove_cvref_t<_Closure>> &&
+                 constructible_from<decay_t<_Closure>, _Closure> &&
+                 constructible_from<decay_t<_OtherClosure>, _OtherClosure>
+    [[nodiscard]] _LIBCPP_HIDE_FROM_ABI
----------------
```
constructible_from<decay_t<_Closure>, _Closure> &&
constructible_from<decay_t<_OtherClosure>, _OtherClosure>
```
should be rewritten as
```
move_constructible<decay_t<_Closure>> && move_constructible<decay_t<_OtherClosure>>
```
but I don't think that's what we want anyway.

================
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)))
----------------
ldionne wrote:
> 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?
Let's keep it at `invocable` for now, and if I can come up with a proof before the branch point, we can change it then.

================
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)); }
----------------
Quuxplusone wrote:
> 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.)
Great, I think we have synergy on this topic! As an aside, I'd like to write a paper proposing turning all stdlib operators as free functions into hidden friends, but that requires a lot of time that I don't have.

================
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)); }
----------------
ldionne wrote:
> 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.
I have a bad feeling about the diagnostics this is gonna produce. Hopefully I'm wrong.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107098