[libcxx-commits] [PATCH] D60368: Add bind_front function (P0356R5)

[Zoe Carver via Phabricator via libcxx-commits]

zoecarver added inline comments.


================
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) :
----------------
ldionne wrote:
> If you used fold a expression like `(is_move_constructible<_Bound>::value && ...)`, I believe you could get rid of `__all_true` completely.
I think I'm going to update this to use the following which will also allow me to get rid of `__all_true`:

```
std::is_move_constructible<std::tuple<_Bound...>>::value
```


================
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>>>>
----------------
ldionne wrote:
> 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.
I think you're right. I'll update it.


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

https://reviews.llvm.org/D60368