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

[Louis Dionne via Phabricator via libcxx-commits]

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