[libcxx-commits] [PATCH] D107199: [libc++] Refactor __perfect_forward, bind_front and not_fn

[Louis Dionne via Phabricator via libcxx-commits]

ldionne added inline comments.


================
Comment at: libcxx/include/__functional/bind_front.h:28
+struct __bind_front_op {
+    template<class ..._Args>
+    _LIBCPP_HIDE_FROM_ABI
----------------
Quuxplusone wrote:
> I'd call this whitespace wrong. (Did clang-format do this?) `template<class... Ts>` is better than `template<class ...Ts>`.
We're quite inconsistent about this in the library. I used `template<...>` because I think I've seen it more often and wanted to be consistent, but I agree `template <...>` is better. Changed.


================
Comment at: libcxx/include/__functional/bind_front.h:31-33
     noexcept(noexcept(_VSTD::invoke(_VSTD::forward<_Args>(__args)...)))
     -> decltype(      _VSTD::invoke(_VSTD::forward<_Args>(__args)...))
     { return          _VSTD::invoke(_VSTD::forward<_Args>(__args)...); }
----------------
mclow.lists wrote:
> Quuxplusone wrote:
> > FWIW, I'd prefer to see lines 31-33 all indented one level (4 spaces).
> > Alternatively, would it make sense to pull `_LIBCPP_EXPRESSION_EQUIVALENT` into `<__config>` or `<type_traits>` and then be able to use it here and below?
> Historical note: I thought several times about writing something like `_LIBCPP_EXPRESSION_EQUIVALENT` for use with `less`, `greater`, etc., and always decided that it reduced the readability of the code.
> 
I'm going to shed the discussion on `_LIBCPP_EXPRESSION_EQUIVALENT` for now in order to make progress. If we want to use it, we can do so in a trivial follow-up patch.


================
Comment at: libcxx/include/__functional/perfect_forward.h:42
 
-    template<class... _Args>
-    _LIBCPP_INLINE_VISIBILITY constexpr auto operator()(_Args&&... __args) &
-    noexcept(noexcept(_Op::__call(_VSTD::get<_Idxs>(__bound_)..., _VSTD::forward<_Args>(__args)...)))
-    -> decltype(      _Op::__call(_VSTD::get<_Idxs>(__bound_)..., _VSTD::forward<_Args>(__args)...))
-    {return           _Op::__call(_VSTD::get<_Idxs>(__bound_)..., _VSTD::forward<_Args>(__args)...);}
-
-    template<class... _Args>
-    _LIBCPP_INLINE_VISIBILITY constexpr auto operator()(_Args&&... __args) const&
-    noexcept(noexcept(_Op::__call(_VSTD::get<_Idxs>(__bound_)..., _VSTD::forward<_Args>(__args)...)))
-    -> decltype(      _Op::__call(_VSTD::get<_Idxs>(__bound_)..., _VSTD::forward<_Args>(__args)...))
-    {return           _Op::__call(_VSTD::get<_Idxs>(__bound_)..., _VSTD::forward<_Args>(__args)...);}
-
-    template<class... _Args>
-    _LIBCPP_INLINE_VISIBILITY constexpr auto operator()(_Args&&... __args) &&
-    noexcept(noexcept(_Op::__call(_VSTD::get<_Idxs>(_VSTD::move(__bound_))...,
-                                  _VSTD::forward<_Args>(__args)...)))
-    -> decltype(      _Op::__call(_VSTD::get<_Idxs>(_VSTD::move(__bound_))...,
-                                  _VSTD::forward<_Args>(__args)...))
-    {return           _Op::__call(_VSTD::get<_Idxs>(_VSTD::move(__bound_))...,
-                                  _VSTD::forward<_Args>(__args)...);}
-
-    template<class... _Args>
-    _LIBCPP_INLINE_VISIBILITY constexpr auto operator()(_Args&&... __args) const&&
-    noexcept(noexcept(_Op::__call(_VSTD::get<_Idxs>(_VSTD::move(__bound_))...,
-                                  _VSTD::forward<_Args>(__args)...)))
-    -> decltype(      _Op::__call(_VSTD::get<_Idxs>(_VSTD::move(__bound_))...,
-                                  _VSTD::forward<_Args>(__args)...))
-    {return           _Op::__call(_VSTD::get<_Idxs>(_VSTD::move(__bound_))...,
-                                  _VSTD::forward<_Args>(__args)...);}
-
-    template<class _Fn = typename tuple_element<0, tuple<_Bound...>>::type,
-             class = _EnableIf<is_copy_constructible_v<_Fn>>>
-    constexpr __perfect_forward_impl(__perfect_forward_impl const& __other)
-        : __bound_(__other.__bound_) {}
-
-    template<class _Fn = typename tuple_element<0, tuple<_Bound...>>::type,
-             class = _EnableIf<is_move_constructible_v<_Fn>>>
-    constexpr __perfect_forward_impl(__perfect_forward_impl && __other)
-        : __bound_(_VSTD::move(__other.__bound_)) {}
-
-    template<class... _BoundArgs>
-    explicit constexpr __perfect_forward_impl(_BoundArgs&&... __bound) :
-        __bound_(_VSTD::forward<_BoundArgs>(__bound)...) { }
+    template<class _Tuple, class ..._Args, class = decltype(_Op::__call(_VSTD::get<_Idx>(declval<_Tuple>())..., declval<_Args>()...))>
+    static auto __can_call_impl(int) -> true_type;
----------------
zoecarver wrote:
> zoecarver wrote:
> > This makes me wonder if we should replace `__call` with `operator()` and then we can just use `is_invocable` here. I guess we could really do that anyway, though. 
> Concepts would make a lot of this a lot easier :(
Yeah, I think it does work actually. Please take a look at my new implementation, I think it's much simpler now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107199