[libcxx-commits] [PATCH] D107098: [libc++] Implement the underlying mechanism for range adaptors
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Tue Aug 17 06:38:18 PDT 2021
ldionne added inline comments.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:36
+template <class _Tp>
+concept _RangeAdaptorClosure = derived_from<decay_t<_Tp>, __range_adaptor_closure<decay_t<_Tp>>>;
+
----------------
Quuxplusone wrote:
> `remove_cvref_t`, not `decay_t`, surely? (Although if `decay_t` is suspected to be faster because compiler builtins, I'll acquiesce instantly.)
Changed. They were using `decay_t` in the paper, but I agree it makes more sense to use `remove_cv_t`.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:36
+template <class _Tp>
+concept _RangeAdaptorClosure = derived_from<_Tp, __range_adaptor_closure<_Tp>>;
+
----------------
Quuxplusone wrote:
> cjdb wrote:
> > I think `__pipeable` is a better name in this case.
> +1 for at least something snake-case. (I wouldn't object to `__pipeable`, although it doesn't capture what's going on very well IMHO.) Basically, I don't like the look of the template parameter `_RangeAdaptorClosure _Closure` on line 45: concepts should have snake-case names. `__range_adaptor_closure _Closure` would be better, but you've already gobbled up that name for the class template itself.
> Alternatively, abandon the terse syntax (abandon the need for a concept representing the class template) and just write
> ```
> template <ranges::viewable_range _View, class _Closure>
> requires __is_range_adaptor_closure<_Closure>
> ```
> Alternatively alternatively, is @cjdb right that these could just be hidden friends of the `_Closure` class? I could imagine that leading to surprising ambiguities when LHS and RHS are both closure classes, so it might need some fiddling-with; but hidden friends would certainly be nice to have.
I am OK with moving the `decay_t` (well, `uncvref_t`) to the concept, however I don't think it makes sense to rename the concept to `__pipeable` if we keep the helper that makes piping available named `__range_adaptor_closure`. In other words, either we have
```
template <class _Tp>
struct __range_adaptor_closure { };
template <class _Tp>
concept _RangeAdaptorClosure = ...;
```
or we have
```
template <class _Tp>
struct __pipeable { };
template <class _Tp>
concept _Pipeable = ...;
```
Otherwise, it seems to me like we're just using inconsistent naming. I'd rather stick with `_RangeAdaptorClosure` for three reasons:
1. http://wg21.link/p2387 uses that naming
2. Being "pipeable" is more general than being a range adaptor closure. One could imagine a pipeable concept describing any type for which `operator|` is defined, but in reality `_RangeAdaptorClosure` describes something more narrow.
3. I think we'll get better diagnostics if we use a concept here than if we use an "opaque" `__is_range_adaptor_closure` `constexpr bool` variable template.
Alternatively, I'd be fine with naming this concept `__is_range_adaptor_closure`, but @cjdb didn't like having `__is` in a concept name (and I agree, but we've got a naming conflict to resolve so that would be an OK workaround for me).
================
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:
> 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.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:48
+_LIBCPP_HIDE_FROM_ABI
+constexpr auto operator|(_View&& __view, _Closure&& __closure)
+noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Closure>(__closure), _VSTD::forward<_View>(__view))))
----------------
cjdb wrote:
> `a | b;` is always a bug for views and is pretty common, not only for folks who are just starting out, but also experienced users too.
Isn't this valid?
```
std::vector<int> ints = {...};
ints | views::transform([](int i) { std::cout << i; return i + 1; }); // intentionally discard the result
```
Someone using `transform` like that would never pass my code review, but it's valid and I think it's too bold to make that ill-formed.
================
Comment at: libcxx/include/__ranges/range_adaptor.h:30
+template <class _Tp>
+struct __range_adaptor_closure { };
+
----------------
cjdb wrote:
> ldionne wrote:
> > Quuxplusone wrote:
> > > zoecarver wrote:
> > > > Why does this need to be a template?
> > > Probably because EBO, specifically the "empty-base exclusion principle." You don't want to have multiple types A,B with the same empty base type, especially when A might have B as its first member.
> > > ( https://quuxplusone.github.io/blog/2021/05/07/std-iterator-as-a-base-class/ )
> > There's what Arthur says, and also I'm sticking closely to http://wg21.link/p2387 so that it's trivial to implement once/if it gets voted in.
> A heads up: I expect P2387 to undergo a few revisions before it gets to LEWG, let alone LWG, so we shouldn't marry ourselves to P2387R0.
Yup - we're not married to it, only using the same mechanism since IMO that's the best way of doing this anyways.
================
Comment at: libcxx/include/__ranges/transform_view.h:404-409
} // namespace ranges
+namespace views {
+namespace __transform {
+ struct __fn {
+ template<ranges::viewable_range _Range, class _Fn>
----------------
cjdb wrote:
> There's not really a point in us doing the checks twice, but if you're wanting to do them, we should probably do them properly.
So the reason why I added this constraint it because it's mentioned in http://eel.is/c++draft/range.adaptors#range.adaptor.object-2.
I am a bit ambivalent. I'll remove the constraint altogether, and if someone gets an error, it should mention why `ranges::transform_view(...)` is ill-formed.
================
Comment at: libcxx/test/std/ranges/range.adaptors/range.transform/types.h:132-142
+struct Twice {
+ constexpr int operator()(int x) { return x + x; }
+};
+
struct Increment {
constexpr int operator()(int x) { return x + 1; }
};
----------------
Quuxplusone wrote:
> Is `Twice` non-const-callable on purpose?
> Tangent: Since callables should generally be const-callable (especially stateless callables), I'd actually prefer to see `Increment` renamed to something like `PlusOneMutable` and `IncrementConst` renamed to just `PlusOne`. Give the weird name to the weird callable.
> Also, if you rename to `PlusOne`, then `Twice` should become `TimesTwo` to match it (and return `x * 2`, too).
Done in 9de882fdbf7436c9ddd9b35b335ec91a524353a5.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D107098/new/
https://reviews.llvm.org/D107098
More information about the libcxx-commits
mailing list