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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added inline comments.


================
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)),
----------------
`__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...


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:297
+        auto& __data   = __storage_.__trivially_destructible_;
+        __data.__call_ = std::bit_cast<void*>(&_FuncWraps::__call);
+        std::construct_at(reinterpret_cast<_UnRefFunc*>(__data.__data_), std::forward<_Func>(__func));
----------------
Not sure if this idea is blessed by the standard or undefined behavior, but:

After changing `operator()` to append instead of prepend the pointer to the functor, this could be rewritten to `std::bit_cast<void*>(static_cast<UnrefFunc>(_func));`, i.e. we could directly store the function pointer inside `__call_` instead of a pointer to the `_FuncWraps::__call` dispatch function.

The `operator()` would call this function pointer with one additional argument. But at on the x64 calling conventions, it is fine to call a function with additional parameters. Additional parameters are simply ignored...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130631