[libcxx-commits] [PATCH] D130631: [libc++] Implement P0288R9 (move_only_function)

[Zhihao Yuan via Phabricator via libcxx-commits]

lichray requested changes to this revision.
lichray added inline comments.


================
Comment at: libcxx/include/__functional/move_only_function.h:26
+#  define _LIBCPP_MOVE_ONLY_FUNCTION_REF &
+#  include <__functional/move_only_function_impl.h>
+
----------------
We should aim for a macro-free implementation to benefit users.


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:70
+  using _DestroyFn = void(void*);
+  using _CallFn    = void;
+
----------------
In his CppCon 2017 talk, Louis @ldionne showed that embedding a vtable in the object storage is not an optimization compared to using a vptr even if we ignore the fact that embedded vtable uses more space. Also, mo-func of 6 x pointers in size may be too large. It's even larger than Swift's existential container (3-pointers + metadata + vptr).


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:203
+    _LIBCPP_HIDE_FROM_ABI static _ReturnT
+    __call(void* __functor, _ArgTypes&&... __args) noexcept(_LIBCPP_MOVE_ONLY_FUNCTION_NOEXCEPT) {
+      return std::invoke(static_cast<_Functor _LIBCPP_MOVE_ONLY_FUNCTION_INV_QUALS>(*static_cast<_Functor*>(__functor)),
----------------
avogelsgesang wrote:
> avogelsgesang wrote:
> > `__functor` should probably be the last argument.
> > 
> > This function forwards all its arguments except for `__functor`.
> > Removing an argument from the front of the argument list means that all parameters must be "shifted" between the registers/on the stack.
> > If instead `functor` is the last argument, no arguments need to be moved around.
> > 
> > See https://godbolt.org/z/hnbe1xYdf
> > 
> > This is also beneficial for `operator()`: `operator()` has to add the `functor` to the argument list. Appending an argument is trivial, but prepending is expensive...
> nevermind - on 2nd thought: this code is only removing the first argument if `__function` is a function pointer. If it is, e.g., a lambda then the first argument, i.e. the `this` pointer, will be replaced but all other arguments stay in their original position.
> 
> Similarly, `operator()` does not actually prepend an argument, but just replaces the first argument, i.e. the `this` pointer
> Similarly, `operator()` does not actually prepend an argument, but just replaces the first argument, i.e. the `this` pointer

Exactly.


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:203
+    _LIBCPP_HIDE_FROM_ABI static _ReturnT
+    __call(void* __functor, _ArgTypes&&... __args) noexcept(_LIBCPP_MOVE_ONLY_FUNCTION_NOEXCEPT) {
+      return std::invoke(static_cast<_Functor _LIBCPP_MOVE_ONLY_FUNCTION_INV_QUALS>(*static_cast<_Functor*>(__functor)),
----------------
lichray wrote:
> avogelsgesang wrote:
> > avogelsgesang wrote:
> > > `__functor` should probably be the last argument.
> > > 
> > > This function forwards all its arguments except for `__functor`.
> > > Removing an argument from the front of the argument list means that all parameters must be "shifted" between the registers/on the stack.
> > > If instead `functor` is the last argument, no arguments need to be moved around.
> > > 
> > > See https://godbolt.org/z/hnbe1xYdf
> > > 
> > > This is also beneficial for `operator()`: `operator()` has to add the `functor` to the argument list. Appending an argument is trivial, but prepending is expensive...
> > nevermind - on 2nd thought: this code is only removing the first argument if `__function` is a function pointer. If it is, e.g., a lambda then the first argument, i.e. the `this` pointer, will be replaced but all other arguments stay in their original position.
> > 
> > Similarly, `operator()` does not actually prepend an argument, but just replaces the first argument, i.e. the `this` pointer
> > Similarly, `operator()` does not actually prepend an argument, but just replaces the first argument, i.e. the `this` pointer
> 
> Exactly.
1. `_ArgTypes&&` - should consider passing objects by copy rather than references as an optimization in certain cases.
2. Please make sure that we don't have a design that works like the following: when `move_only_function` is initialized from a pointer to function, we need to dereference `__functor` first to reach the function pointer, and then make a call via that function pointer.


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:267
+      __storage_.__large_.__destroy_(__storage_.__large_.__heap_);
+      ::operator delete(__storage_.__large_.__heap_);
+      break;
----------------
It seems that some logic is embedded in the mo-func body, rather than dispatched. `__destroy_` is not sufficiently used.


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:359
+      noexcept(_LIBCPP_MOVE_ONLY_FUNCTION_NOEXCEPT) {
+    auto __call = [&](auto&& __callable, auto&& __data) {
+      return std::bit_cast<void (*)(_LIBCPP_MOVE_ONLY_FUNCTION_CV void*, _ArgTypes...)>(__callable)(
----------------
Please make sure the type is ABI-compatible after adopting https://wg21.link/p2511/github. It is on the way to C++26.


================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.move/call/normal_const.pass.cpp:83
+    assert(f);
+    LIBCPP_ASSERT(f.__get_status() == std::__move_only_function_storage::_Status::_TriviallyDestructible);
+  }
----------------
We should test against behaviors, not guts. If the difference is not observable according to the standard, consider observing some extension such as a more strict `noexcept`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130631