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

[Nikolas Klauser via Phabricator via libcxx-commits]

philnik added inline comments.


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:70
+  using _DestroyFn = void(void*);
+  using _CallFn    = void;
+
----------------
lichray wrote:
> philnik wrote:
> > 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.
> According to STL the person, it is reasonable to "store a `std::string` locally." To libc++, that's 24 bytes; 32 bytes for the whole mv-func object with a vptr.
We've got multiple threads discussing the this, so I'm closing some.


================
Comment at: libcxx/include/__functional/move_only_function_impl.h:251
+  static constexpr bool __fits_in_buffer_impl =
+      is_trivially_move_constructible_v<_Func> && is_trivially_destructible_v<_Func> &&
+      sizeof(_Func) <= __buffer_size_ && alignof(_Func) <= alignof(void*);
----------------
ldionne wrote:
> I think we should make it possible to store non-trivially-fooable types in the small buffer. IIUC you only need that constraint in order to implement `swap`, but you should be able to handle that by adding yet another function to the vtable. This would create a size issue right now, but wouldn't be a concern anymore if we move to a remote vtable.
The question of using the SBO for non-trivially relocatable types is spanning multiple threads, so I'm closing this one.


================
Comment at: libcxx/include/__utility/small_buffer.h:38
+  template <class _Tp>
+  static constexpr bool __fits_in_buffer_impl =
+      is_trivially_move_constructible_v<_Tp> && is_trivially_destructible_v<_Tp> && sizeof(_Tp) <= _BufferSize &&
----------------
ldionne wrote:
> I think we should allow storing non-trivially movable types in the SBO. This would require another "virtual" function to perform the move, but I think that's worth the trouble. Concretely, the size of the vtable isn't a huge issue.
This would also make `move_only_function` not trivially relocatable anymore.


================
Comment at: libcxx/include/__utility/small_buffer.h:56
+  template <class _Stored>
+  _LIBCPP_HIDE_FROM_ABI _Stored* __get() {
+    if constexpr (__fits_in_buffer<_Stored>)
----------------
ldionne wrote:
> Do we need `launder`?
I //think// the answer is yes, because we might never have destroyed a trivial object that was in `__buffer_` before, but I'm not familiar enough with the technicalities of object lifetime to be confident in answering the question.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130631