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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik marked 9 inline comments as done.
philnik 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>
+
----------------
lichray wrote:
> We should aim for a macro-free implementation to benefit users.
If you have an idea on how to do that I'd be happy to try it, but I don't think writing the implementation 12 times is worth removing the macros.


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:122
+
+    __call_ = &__function_wrappers<_Func>::__call;
+    if constexpr (!is_trivially_destructible_v<decay_t<_Func>>) {
----------------
Can the indirection be avoided for function pointers?


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:245
+
+  _CallFn* __call_       = nullptr;
+  _DestroyFn* __destroy_ = nullptr;
----------------
Take a look at https://wg21.link/p2511


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:70
+  using _DestroyFn = void(void*);
+  using _CallFn    = void;
+
----------------
lichray wrote:
> 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).
Do you have any reasoning how large it should be? It's simple to change, since I don't know what the size should be.


================
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)(
----------------
lichray wrote:
> Please make sure the type is ABI-compatible after adopting https://wg21.link/p2511/github. It is on the way to C++26.
I think the only things still relevant from this thread are P2511 and avoiding the indirection for function pointers. To make it simpler I added comments independent from this thread for these things.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130631