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

[Arthur O'Dwyer via Phabricator via libcxx-commits]

Quuxplusone added a comment.

(Phab is listing several of my comments as "unsubmitted" but I think that's just a glitch. Sorry if I'm resubmitting comments and/or some of these comments look obsolete now.)



================
Comment at: libcxx/include/__functional/bind_front.h:28
+struct __bind_front_op {
+    template<class ..._Args>
+    _LIBCPP_HIDE_FROM_ABI
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > 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.
> > Heh, personally I prefer `template<` over `template <` (although the latter is libc++ style), so "agree" is the wrong word. :)
> > 
> > But I was talking about `template <class... _Args>` versus `template <class ..._Args>`. You've still got the latter on line 28. I'd like to see the former.
> Oops, I read that too fast.
> 
> Regarding `template <class... _Args>` versus `template <class ..._Args>`: When an ellipse appears to the left of a parameter, it declares a parameter pack. When placed to the right, it expands the parameter pack (see https://docs.microsoft.com/en-us/cpp/cpp/ellipses-and-variadic-templates?view=msvc-160#syntax). I've always used the convention of declaring parameter packs with `...args` because it follows most naturally from that. Using `class... Args` makes it look like we're expanding the parameter pack `class`, which is not what's happening. Of course, it's all just whitespace and both ways are equivalent, but IMO using `class ...Args` conveys better what we're doing based on the syntax of the language.
I remember thinking that way back in the 2012-2015 timeframe; but these days `class... Ts` is just so much easier on the eyes; and it's certainly libc++ style. The syntax situation is of course exactly analogous to `const _Tp& __t` versus `const _Tp &__t`, or for that matter `_Tp const &__t`: there's "the way everyone actually likes to see it" and then "the way that reflects the history of the syntax."


================
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;
----------------
ldionne wrote:
> Quuxplusone wrote:
> > ldionne wrote:
> > > 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.
> > Per my "When Should You Give Two Things the Same Name?", you know I'm leery of giving this detail the same name as the ordinary call operator. Do we ever publicly inherit from `__perfect_forward_impl`, or do anything else where the user-programmer might observe that `foo(1,2,3)` works when it shouldn't?
> > (Whereas the user-programmer is not allowed to observe that `foo.__call(1,2,3)` works, because `__call` is a reserved name.)
> > However, I'm not asking for it to be changed back //unless// someone (not me) comes up with the actual concrete example that proves why the name `operator()` can't be used. I'm just betting that such an example exists.
> Just for the record, I didn't change `__perfect_forward::__call` to `__perfect_forward::operator()`. It's always been `operator()`, and that's important because the intent is for it to be inherited and provide its base class with an `operator()`.
> 
> The change was to go from `_Op::__call` to `_Op::operator()` in order to be able to reuse `std::is_invocable` instead of rolling our own. But `_Op` is an internal-only helper and we never inherit from it.
Ah. SGTM then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107199