[libcxx-commits] [PATCH] D60368: Add bind_front function (P0356R5)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Mon Sep 14 13:07:31 PDT 2020
ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.
I'm not seeing any issues with the perfect forwarding anymore. This looks pretty good, with a few comments.
================
Comment at: libcxx/include/functional:2923
+ template<class... _Args>
+ constexpr auto operator()(_Args&&... __args) &
+ noexcept(noexcept(_Op::__call(_VSTD::get<_Idxs>(__bound)..., _VSTD::forward<_Args>(__args)...)))
----------------
Can you please throw in `_LIBCPP_INLINE_VISIBILITY` (and below)?
================
Comment at: libcxx/include/functional:2964
+ class = _EnableIf<is_constructible<_VSTD::tuple<_Bound...>, _BoundArgs...>::value &&
+ __all_true<is_move_constructible<_Bound>::value...>::value>>
+ explicit constexpr __perfect_forward_impl(_BoundArgs&&... __bound) :
----------------
If you used fold a expression like `(is_move_constructible<_Bound>::value && ...)`, I believe you could get rid of `__all_true` completely.
================
Comment at: libcxx/include/functional:3004
+template<class _Fn, class... _Args,
+ class = _EnableIf<is_constructible_v<decay_t<_Fn>, _Fn> &&
+ is_move_constructible_v<decay_t<_Fn>>>>
----------------
I think you need to check all the requirements at this point (from the paper):
```
conjunction_v<is_constructible<FD, F>, is_move_constructible<FD>, is_constructible<BoundArgs, Args>..., is_move_constructible<BoundArgs>...>
```
As it stands, you seem to be testing for `is_constructible<FD, F> && is_move_constructible<FD>` here, but you're delaying the checks for `is_constructible<BoundArgs, Args>` and `is_move_constructible<BoundArgs>` until the constructor of `__perfect_forward_impl`. I think you need to check everything eagerly here, or some SFINAE-able errors may become hard errors instead.
Furthermore, I would suggest going the easy way and sticking to the way the spec words it: you could just use `conjunction` instead of fold expressions or other clever tricks above -- this will also have the correct short-circuit behavior for evaluating the various `is_constructible<BoundArgs, Args>` and `is_move_constructible<BoundArgs, Args>` traits.
================
Comment at: libcxx/include/functional:3139
+ for (typename _Container::iterator __iter = __c.begin(), __last = __c.end(); __iter != __last;)
+ {
+ if (__pred(*__iter))
----------------
Can you please drop these unrelated whitespace changes?
================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.fail.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
You could rename this to `.verify.cpp` instead since it's using clang-verify.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D60368/new/
https://reviews.llvm.org/D60368
More information about the libcxx-commits
mailing list