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

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

Quuxplusone accepted this revision as: Quuxplusone.
Quuxplusone added a comment.

If buildkite is happy, I'm happy.



================
Comment at: libcxx/include/__functional/bind_front.h:28
+struct __bind_front_op {
+    template<class ..._Args>
+    _LIBCPP_HIDE_FROM_ABI
----------------
I'd call this whitespace wrong. (Did clang-format do this?) `template<class... Ts>` is better than `template<class ...Ts>`.


================
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)...); }
----------------
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?


================
Comment at: libcxx/include/__functional/not_fn.h:30-32
     noexcept(noexcept(!_VSTD::invoke(_VSTD::forward<_Args>(__args)...)))
     -> decltype(      !_VSTD::invoke(_VSTD::forward<_Args>(__args)...))
     { return          !_VSTD::invoke(_VSTD::forward<_Args>(__args)...); }
----------------
Again FWIW I'd prefer 4 spaces in front of lines 30-32.


================
Comment at: libcxx/include/__functional/perfect_forward.h:82
+
+    template<class ..._Args, class = _EnableIf<__can_call<tuple<_Bound...>, _Args...>>>
+    _LIBCPP_HIDE_FROM_ABI constexpr auto operator()(_Args&&... __args) &&
----------------
zoecarver wrote:
> Shouldn't tuple be an xvalue here, not a prvalue? Or am I just confused?
The trick is that `declval<_Tuple>()` on line 42 always returns `_Tuple&&`. So adding another `&&` here on line 82 would be harmless but also unnecessary.


================
Comment at: libcxx/include/__functional/perfect_forward.h:103
+
+// __perfect_forward implements a perfect-forwarding call wrapper as explained in http://eel.is/c++draft/func.require.
+template<class _Op, class ..._Args>
----------------
Nit: I'd make this just `[func.require]`, not a full URL.


================
Comment at: libcxx/test/std/utilities/function.objects/func.bind_front/bind_front.pass.cpp:397-402
 int main(int, char**) {
-  test();
-  static_assert(test());
+  tests();
+  static_assert(tests());
 
   return 0;
 }
----------------
(1) Surely `s/tests/test/`?
(2) I notice a lack of tests verifying no-special-treatment-of-`std::reference_wrapper`-arguments. Maybe consider adding one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107199