[libcxx-commits] [PATCH] D130631: [libc++] Implement P0288R9 (move_only_function)
Louis Dionne via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Dec 15 10:53:43 PST 2022
ldionne requested changes to this revision.
ldionne added inline comments.
This revision now requires changes to proceed.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:39-54
+#ifndef _LIBCPP_MOVE_ONLY_FUNCTION_CV
+# define _LIBCPP_MOVE_ONLY_FUNCTION_CV
+#endif
+
+#ifndef _LIBCPP_MOVE_ONLY_FUNCTION_REF
+# define _LIBCPP_MOVE_ONLY_FUNCTION_REF
+# define _LIBCPP_MOVE_ONLY_FUNCTION_INVOKE_QUALS _LIBCPP_MOVE_ONLY_FUNCTION_CV&
----------------
Is there a reason why you allow these macros to *not* be defined? I would simply document the macros that this file needs in a comment and also have a `#ifndef #error #endif` for each of them.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:67
+
+template <class _ReturnT, class... _ArgTypes>
+class _LIBCPP_MOVE_ONLY_FUNCTION_TRIVIAL_ABI move_only_function<_ReturnT(
----------------
Please add documentation explaining the design choices we made and why. Basically a summary of this review. You'll thank yourself in a few months/years -- it's much much easier to do it right now while it's fresh.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:76
+ template <class _Functor>
+ struct __function_wrappers_impl {
+ __function_wrappers_impl() = delete;
----------------
This doesn't look super useful anymore.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:101
+ template <class _Functor>
+ static constexpr _TrivialVTable __trivial_vptr_ = {.__call_ = __function_wrappers<_Functor>::__call};
+
----------------
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:103
+
+ template <class _Functor>
+ static constexpr _NonTrivialVTable __non_trivial_vptr_{
----------------
I would call those `__trivial_vtable` and `__nontrivial_vtable` instead, since they are vtables and not vpointers.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:104-107
+ static constexpr _NonTrivialVTable __non_trivial_vptr_{
+ {.__call_ = __function_wrappers<_Functor>::__call},
+ __function_wrappers<_Functor>::__destroy,
+ };
----------------
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:110
+ template <class _Functor>
+ __pointer_bool_pair<const _TrivialVTable*> __get_vptr() {
+ if constexpr (_BufferT::__fits_in_buffer<_Functor> && is_trivially_destructible_v<_Functor>) {
----------------
`_LIBCPP_HIDE_FROM_ABI` here and wherever else you need it -- please make a pass.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:130
+ template <class _VT>
+ static constexpr bool __is_callable_from = __is_callable_from_impl<decay_t<_VT>>();
+
----------------
You could use an immediately-invoked lambda here to avoid defining a separate function, since it's used exactly once.
================
Comment at: libcxx/include/__functional/move_only_function_impl.h:61
+# define _LIBCPP_MOVE_ONLY_FUNCTION_TRIVIAL_ABI
+#endif
+
----------------
ldionne wrote:
> https://wg21.link/p2548 `copyable_function` is making progress through the committee. We should discuss how these two types are going to interoperate and whether that should inform the representation we choose for `move_only_function`. In particular, it would be a nice QOI property to have `move_only_function(copyable_function&&)` just steal the guts of the `copyable_function` and make no allocation.
I think the question here is basically whether we will want to have non-trivially-movable types in the SBO of `copyable_function`. If we do, then we won't be able to steal the guts, otherwise we will (assuming we use a SBO size that is no larger than that of move-only-function).
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
This one in a separate patch too, please!
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:28-30
+// A __pointer_int_pair is a pair of a pointer and an integral type. The integral type is stored inside the lower bits
+// of the pointer, which allows the whole pair to be as large as only the pointer. To achieve that the pointed-to type
+// has to have an alignment requirement of at least the amount of bits which should be used for the integral type.
----------------
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:36
+ requires is_pointer_v<_Pointer>
+struct _PointerLikeTraits {
+ static constexpr size_t __low_bits_available = std::__log2i(alignof(remove_pointer_t<_Pointer>));
----------------
We should implement `_PointerLikeTraits` for `__pointer_int_pair` so that we can chain `__pointer_int_pair`s.
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:47
+ size_t __int_bit_count,
+ class _IntType = size_t,
+ class _PointerTraits = _PointerLikeTraits<_Pointer>>
----------------
Let's `require std::is_signed_v` unless you want to have some fun making sure it works with signed types.
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:49
+ class _PointerTraits = _PointerLikeTraits<_Pointer>>
+class __pointer_int_pair {
+ static_assert(__int_bit_count <= _PointerTraits::__low_bits_available,
----------------
Can you please make sure that you have tests with a fancy pointer class?
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:58
+
+public:
+ _LIBCPP_HIDE_FROM_ABI __pointer_int_pair(nullptr_t) {}
----------------
Lets explicitly do `__pointer_int_pair(__pointer_int_pair const&) = default`, and so on for the move constructor and the assignment operators. It's nice documentation to state that it's trivially whatever-able just like one would expect.
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:59
+public:
+ _LIBCPP_HIDE_FROM_ABI __pointer_int_pair(nullptr_t) {}
+ _LIBCPP_HIDE_FROM_ABI __pointer_int_pair(_Pointer __ptr_value, _IntType __int_value)
----------------
`constexpr`? That will make it constinit-able.
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:62
+ : __value_(reinterpret_cast<uintptr_t>(__ptr_value) | (__int_value << __extra_bits)) {
+ _LIBCPP_ASSERT((__int_value & __int_mask) == __int_value, "integer is too large!");
+ _LIBCPP_ASSERT((reinterpret_cast<uintptr_t>(__ptr_value) & __ptr_mask) == reinterpret_cast<uintptr_t>(__ptr_value),
----------------
I think this assertion is incorrect. Let's say you have something like this:
```
// int value: 15 aka 1111 binary
// number of bits free in the pointer: 4
// number of bits in use: 2
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxx 0000 // before you store anything into the pointer
xxxxxxxxxxxxxxxxxxxxxxxxxxxx11 1100 // after you store 15 into the pointer (it overflowed)
xxxxxxxxxxxxxxxxxxxxxxxxxxxx11 1111 // assuming you were using the 2 lower bits of the pointer to store something like int-value=3
```
Your assertion will pass but you still overflowed.
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:67
+
+ _LIBCPP_HIDE_FROM_ABI __pointer_int_pair& operator=(nullptr_t) {
+ __value_ = 0;
----------------
`constexpr`
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:72
+
+ _LIBCPP_HIDE_FROM_ABI _IntType __get_value() const { return (__value_ & __int_mask) >> __extra_bits; }
+
----------------
This could be `constexpr`?
================
Comment at: libcxx/include/__utility/pointer_int_pair.h:76-78
+ _LIBCPP_HIDE_FROM_ABI _Pointer operator->() const { return __get_ptr(); }
+
+ _LIBCPP_HIDE_FROM_ABI bool operator==(nullptr_t) const { return __get_ptr() == nullptr; }
----------------
Looking at this, I am wondering whether we actually want to give it a pointer-like interface at all. This is a pair, not a pointer. Maybe we just want `__first()` and `__second()`? That would simplify the API.
And we should also make it work with structured bindings by making it a tuple-like class.
================
Comment at: libcxx/include/__utility/small_buffer.h:1
+//===----------------------------------------------------------------------===//
+//
----------------
Can you split this class off into its own patch?
================
Comment at: libcxx/include/__utility/small_buffer.h:32-33
+
+template <size_t _BufferSize, size_t _BufferAlignment>
+class _LIBCPP_SMALL_BUFFER_TRIVIAL_ABI __small_buffer {
+ static_assert(_BufferSize > 0, "The buffer size should not be zero");
----------------
~~That way we can decide from `move_only_function` the additional criteria for storing the type in the SBO.~~
Nevermind, this adds too much complexity in the move constructor for little benefit since the only user (for now) would have it be trivially relocatable anyways.
================
Comment at: libcxx/include/__utility/small_buffer.h:33
+template <size_t _BufferSize, size_t _BufferAlignment>
+class _LIBCPP_SMALL_BUFFER_TRIVIAL_ABI __small_buffer {
+ static_assert(_BufferSize > 0, "The buffer size should not be zero");
----------------
Please document the design of the class, in particular the fact that it only ever stores trivially movable objects in the buffer, which allows it to be trivially relocatable. Also mention that the users of the class are expected to manage the lifetime of the object inside the buffer.
================
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 &&
----------------
philnik wrote:
> 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.
So the tradeoff here is that either we accept non-trivially movable types in the SBO, or we only accept trivially movable types and then we can guarantee that the SBO is trivially relocatable.
I think this decision boils down to what's the most important use case for `move_only_function`. In live review, I said I thought it would be used mostly as a function parameter, however you rightly point out that we'll have `function_ref` for that. So for times where we don't want to actually store the `move_only_function`, we can just use `function_ref` and there's never an allocation.
So I think it would make sense to keep it trivially relocatable, as in the current patch.
================
Comment at: libcxx/include/__utility/small_buffer.h:56
+ template <class _Stored>
+ _LIBCPP_HIDE_FROM_ABI _Stored* __get() {
+ if constexpr (__fits_in_buffer<_Stored>)
----------------
philnik wrote:
> 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.
I think we need to launder for this case:
```
struct Foo { };
struct Bar { };
buf.construct<Foo>(Foo{});
Foo* foo = buf.__get<Foo>();
use(buf);
Bar* bar = buf.__get<Bar>();
// somewhere else
void use(__small_buffer& buf) {
buf.construct<Bar>(Bar{});
}
```
================
Comment at: libcxx/test/libcxx/utilities/pointer_int_pair.pass.cpp:9
+
+// UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
+
----------------
Maybe there should be more tests? This is a pretty fundamental utility.
Also please test chaining, etc.
================
Comment at: libcxx/test/std/utilities/function.objects/func.wrap/func.wrap.move/assignment/nullptr.pass.cpp:70
+ test_member_function_pointer<T>();
+ });
+}
----------------
`return 0` everywhere please!
================
Comment at: libcxx/test/support/type_algorithms.h:121
+template <class Func>
+using function_noexcept_const_lvalue_ref_qualified =
+ typename function_noexcept_const_lvalue_ref_qualified_impl<Func>::type;
----------------
Maybe this should take an enum that you can bit-or to say which combinations you want?
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