[libcxx-commits] [PATCH] D130631: [libc++] Implement P0288R9 (move_only_function)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Oct 13 09:36:05 PDT 2022
ldionne requested changes to this revision.
ldionne added a subscriber: EricWF.
ldionne added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/__functional/move_only_function.h:26
+# define _LIBCPP_MOVE_ONLY_FUNCTION_REF &
+# include <__functional/move_only_function_impl.h>
+
----------------
philnik wrote:
> lichray wrote:
> > philnik wrote:
> > > 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.
> > No macro and maintainable. Here is an example: https://github.com/zhihaoy/nontype_functional/blob/main/include/std23/move_only_function.h
> > With C++ explicit object member function, you can also turn the six `operator()` into one.
> I don't think that implementation is conforming. The standard explicitly states that there have to be partial specializations for each combination of cv, ref and noex (http://eel.is/c++draft/func.wrap.move#general-1). I didn't ask before, but how exactly does it affect users whether the implementation is macro-free or not?
I agree with @philnik that this isn't conforming, although I think it's only pedantically non-conforming. In practice, I would expect that it works for most users.
Most importantly, it seems to me like the spec is too narrow here. What the standard *really* wants to say is that `move_only_function` can handle any combination of `cv noexcept ref`, but I don't think it was ever an explicit goal to say that it would be achieved via specialization specifically.
@jwakely @lichray Do you agree this is overspecified? If so, I could write a NB comment about it and we could relax the specification. Otherwise, I would argue that the current approach taken by @philnik is probably the right one, even though I agree it's kind of lame to be required by the Standard to use the X macro idiom.
================
Comment at: libcxx/include/__functional/move_only_function_common.h:31
+template <bool _Noexcept, class _BufferT, class _ReturnT, class... _ArgTypes>
+struct _VTable {
+ using _DestroyFunc = void(_BufferT&) noexcept;
----------------
Let's call this in a way that it's `move_only_function` specific. Maybe `_MoveOnlyFunctionVtable` or whatever. Just `std::_VTable` is taking a pretty general name and assigning it specific meaning, which is weird.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:99
+ static constexpr _VTable __vtable_data_ = {
+ .__destroy_ = is_trivially_destructible_v<_Functor> ? nullptr : __function_wrappers<_Functor>::__destroy,
+ .__call_ = __function_wrappers<_Functor>::__call};
----------------
If we do like `llvm::unique_function` (and I think we should), then we would have two sorts of vtables (pseudocode obviously):
```
struct _TrivialMoveOnlyFunctionVTable {
auto __call_;
};
struct _NonTrivialMoveOnlyFunctionVTable : _TrivialMoveOnlyFunctionVTable {
auto __destroy_;
auto __move_;
};
```
We could use the pointer union trick to store whether the vtable is for a trivial type or not, which would replace your check for `__destroy_ == nullptr` in `__reset()`. IMO that would be slightly better.
We should implement a pointer union like type in libc++, that's something we should have had for a while. Give me a few days to get to it, and if I don't manage to I'll let you know and you can treat yourself.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:158
+ if (__func) {
+ __vtable_ = std::exchange(reinterpret_cast<const _VTable*&>(__func.__vtable_), nullptr);
+ __buffer_ = std::move(__func.__buffer_);
----------------
How do you know that `__func.__vtable_` is compatible with this function's vtable? And by that I mean precisely: how do you know that this function's `vtable.__call_` (and `__destroy_` and `__move_`) have __exactly__ the same signature as `__func.__vtable_`'s? If they are not __exactly the same__, it's going to be UB to call them through pointers to different types.
>From live review: you say this is definitely UB. First, please ensure to call out things like that explicitly. Second, I don't think we should perform this optimization unless we have some sort of blessing from the compiler (through a builtin or something like that).
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:243
+
+ static constexpr size_t __buffer_size_ = 4 * sizeof(void*);
+
----------------
philnik wrote:
> ldionne wrote:
> > IMO this is too large. I believe that types with `sizeof(T) == 32` AND that are trivially move constructible/destructible AND that we want to type erase are going to be quite rare. I think that most types will be either smaller, or will be non-trivial. Shooting from the hip, I would use something like `2*sizeof(void*)` at most, but we definitely need to look at what other implementations are doing, and also what other common utilities like this do (let's do a quick survey in open source libraries, e.g. have Folly or Abseil already shipped something like this, and if so, is there something we can learn from their experience?).
> | library | name | size | buffer size |
> | Abseil | `absl::any_invocable` | 32 | 16 |
> | libstdc++ | `std::move_only_function` | 40 | 24 |
> | folly | `folly::Function` | 64 | 48 |
> | LLVM | `llvm::unique_function` | 32 | 24 |
> | HPX | `hpx::move_only_function` | 40 | 24 |
> | MSVC STL | `std::move_only_function` | 64 | 56 |
>
Shooting from the hip, I would go for an implementation like `llvm::unique_function`, with 32 total size and 24 SBO. But it would be great to have more data about how these types are used. Unfortunately that's pretty difficult to get.
@EricWF Do you have any way to gather some data on your code base that would help us pick something here?
================
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 &&
----------------
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.
================
Comment at: libcxx/include/__utility/small_buffer.h:46
+
+ __small_buffer() = default;
+ __small_buffer(const __small_buffer&) = delete;
----------------
IMO it would be nice if `__small_buffer` had a constructor that took an object. It would then encapsulate way more logic. That does mean that `__small_buffer` would have to know how to allocate and deallocate, but that can be done via template parameters. I think that would simplify the implementation of `move_only_function` quite a bit.
It would also make sense (?) if `__small_buffer` could be swapped, IMO. That also requires additional knowledge that should be parameterizable.
================
Comment at: libcxx/include/__utility/small_buffer.h:56
+ template <class _Stored>
+ _LIBCPP_HIDE_FROM_ABI _Stored* __get() {
+ if constexpr (__fits_in_buffer<_Stored>)
----------------
Do we need `launder`?
================
Comment at: libcxx/include/__utility/small_buffer.h:68
+ } else {
+ byte* __allocation = static_cast<byte*>(::operator new[](sizeof(_Stored), align_val_t{alignof(_Stored)}));
+ std::construct_at(reinterpret_cast<byte**>(__buffer_.data()), __allocation);
----------------
You'll need to guard this for exceptions once you store non-trivial types in the buffer.
================
Comment at: libcxx/include/__utility/small_buffer.h:81
+private:
+ alignas(_BufferAlignment) array<std::byte, _BufferSize> __buffer_;
+};
----------------
This allows dropping the dependency on `std::array`, which doesn't add much. The `_BufferSize` is always `> 0`.
================
Comment at: libcxx/test/libcxx/utilities/function.objects/func.wrap/func.wrap.move/call_disengaged.pass.cpp:1
+//===----------------------------------------------------------------------===//
+//
----------------
Can you call this `assert.<whatever>` for consistency?
================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.move/common.h:15-16
+
+inline bool called;
+inline void call_func() noexcept { called = true; }
+
----------------
Please keep this in each file, even if that means duplicating it. Seeing a variable named `called` in the tests coming from seamingly nowhere is really weird IMO. And the duplication isn't much.
================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.move/common.h:82
+
+ void operator()() const noexcept { called = true; }
+ int operator()(int i) const noexcept { return i; }
----------------
This is weird too - this has a different behavior depending on whether the function returns `void` or not. I understand you were maybe trying to reduce code duplication, but this is kind of spaghetti in return, IMO. Instead, consider always setting a bool to express that the function was called. The only difference will then become whether you're taking an argument or not and what you're returning, which seems better:
```
struct NonTrivial {
NonTrivial() = default;
NonTrivial(MoveCounter, bool* set_on_call) {}
NonTrivial(std::initializer_list<int>&, MoveCounter, bool* set_on_call) {}
NonTrivial(NonTrivial&&) noexcept(false) {} // todo
~NonTrivial() {}
void operator()() const noexcept { *called_ = true; }
int operator()(int i) const noexcept { *called_ = true; return i; }
};
```
Also, I would suggest not using a global here. It's not `constexpr` friendly and it's also not the best style, even though I understand it results in more "compact" code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D130631/new/
https://reviews.llvm.org/D130631
More information about the libcxx-commits
mailing list