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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne marked 3 inline comments as done.
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:
> 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.


================
Comment at: libcxx/include/__functional/perfect_forward.h:44-48
+    __perfect_forward_impl(__perfect_forward_impl const&) = default;
+    __perfect_forward_impl(__perfect_forward_impl&&) = default;
+
+    __perfect_forward_impl& operator=(__perfect_forward_impl const&) = default;
+    __perfect_forward_impl& operator=(__perfect_forward_impl&&) = default;
----------------
miscco wrote:
> Is it necessary to default those? they should be compiler generated anyway
I think I added them because some test was failing, however logic would dictate that they are not necessary. And to be sure, I removed them, ran the tests again and everything succeeded, so we could remove them.

However, I kind of like defaulting them explicitly since it reduces the cognitive burden (for me at least) of checking whether the compiler will generate them or not. LMK if you'd much rather remove them.


================
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;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107199