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

[Adrian Vogelsgesang via Phabricator via libcxx-commits]

avogelsgesang added a comment.

Thanks for looking into this :) `std::move_only_function` is very high on the list of my favorite new C++23 features!



================
Comment at: libcxx/include/__functional/move_only_function_impl.h:280
+  _LIBCPP_HIDE_FROM_ABI move_only_function(move_only_function&& __other) noexcept : __storage_(__other.__storage_) {
+    __other.__storage_ = {};
+  }
----------------
Is it more efficient to only set `__status_ = NotEngaged`? Afaict, this would overwrite only 1 instead of 48 bytes

(here and a couple of other places which set `__storage_ = {}`


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:294
+        __storage_ = {};
+        return;
+      } else {
----------------
redundant `return` before `else`?


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:360
+    auto __call = [&](auto&& __callable, auto&& __data) {
+      return std::bit_cast<void (*)(_LIBCPP_MOVE_ONLY_FUNCTION_CV void*, _ArgTypes...)>(__callable)(
+          __data, std::forward<_ArgTypes>(__args)...);
----------------
`std::bit_cast<__ReturnT (*)(...)>` instead of `std::bit_cast<void (*)(...)>`?

Also, it seems there are no test cases which instantiate `move_only_function` with a return type != `void`?


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:360
+    auto __call = [&](auto&& __callable, auto&& __data) {
+      return std::bit_cast<void (*)(_LIBCPP_MOVE_ONLY_FUNCTION_CV void*, _ArgTypes...)>(__callable)(
+          __data, std::forward<_ArgTypes>(__args)...);
----------------
avogelsgesang wrote:
> `std::bit_cast<__ReturnT (*)(...)>` instead of `std::bit_cast<void (*)(...)>`?
> 
> Also, it seems there are no test cases which instantiate `move_only_function` with a return type != `void`?
I think this should also take `_LIBCPP_MOVE_ONLY_FUNCTION_NOEXCEPT` into account when casting to the function pointer.

As written now, for `move_only_function<void () noexcept>`, this `operator()` will contain a call to a non-noexcept function. The compiler will hence generate additional exception handling code. Also, this blocks tail call optimization (https://godbolt.org/z/39bn95cdT)


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:365
+    using enum _Store::_Status;
+    switch (__storage_.__large_.__status_) {
+    case _NotEngaged:
----------------
I think this can be implemented more efficiently by folding this switch with the indirect function call we are doing here anyway.

**Proposed changes**

Move this switch into `__function_wrappers_impl::__call`, taking `_Store::_Status` as a template parameter:

```
template<_Store::_Status status>
_LIBCPP_HIDE_FROM_ABI static _ReturnT
__call(_Store* store, _ArgTypes&&... __args) noexcept(_LIBCPP_MOVE_ONLY_FUNCTION_NOEXCEPT) {
    auto __doCall = [&](void* __functor) {
        return std::invoke(static_cast<_Functor _LIBCPP_MOVE_ONLY_FUNCTION_INV_QUALS>(*static_cast<_Functor*>(__functor)),
                         std::forward<_ArgTypes>(__args)...);
    }
    using enum _Store::_Status;
    if constexpr (status == _NotEngaged) {
      _LIBCPP_ASSERT(false, "Tried to call move_only_function which doesn't hold a callable object");
    }  else if constexpr (status == _TriviallyDestructible) {
        return __doCall(&__storage_.__trivially_destructible_.__data_);
    }  else if constexpr (status == _TriviallyRelocatable) {
        return __doCall(&__storage_.__trivially_relocatable_.__data_);
    }  else if constexpr (status == _Heap) {
        return __doCall(&__storage_.__large_.__data_);
    }
}
```

In `__construct`, the `__storage.__large_.__call` would then be assigned similar to

```
auto& __data   = __storage_.__large_;
__data.__call_ = std::bit_cast<void*>(&_FuncWraps::__call<_Store::_Status::_Heap>);
```

Because the `__call_` member is always at the same position inside the `__move_only_function_storage`, we can then implement `operator()` as a simple

```
  _LIBCPP_HIDE_FROM_ABI _ReturnT operator()(_ArgTypes... __args) _LIBCPP_MOVE_ONLY_FUNCTION_CV_REF
      noexcept(_LIBCPP_MOVE_ONLY_FUNCTION_NOEXCEPT) {
    return std::bit_cast<ReturnT (*)(_LIBCPP_MOVE_ONLY_FUNCTION_CV  _Store*, _ArgTypes...)>(__storage_.__large_.__call_)(
        &__storage, std::forward<_ArgTypes>(__args)...);
```


**Why I think this is a good idea**

Replacing the `switch` with a `if constexpr` chain guarantees that this switch is completely optimized away.

Futhermore for usages like

```
void setCallback(std::move_only_function<void()> cb);

void setCallbackWithGreetings(bool bye, const char* whom) {
     takesSomeCallback([=]() {
          std::cout << (bye ? "Goodbye " : "Hello ") << whom << "\n";
     });
}
```

the compiler can inline the lambda body into the `__function_wrappers_impl::__call`. The compiler can then fold the offset computation for `__storage_.__large_.__data_` with the offset computations for `whom` and `bye`. With the previous implementation of `operator()` this would not have been possible since the `__storage_.__large_.__data_` happens before the indirect call, while the `whom` & `bye` offset computations happened inside the indirect call.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130631