diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 0ad10e9a93..a58d0929fb 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -50,14 +50,15 @@ PYBIND11_NAMESPACE_BEGIN(detail) template class type_caster : public type_caster_base { }; template using make_caster = type_caster>; -// Shortcut for calling a caster's `cast_op_type` cast operator for casting a type_caster to a T +// Shortcuts for calling a caster's `cast_op_type` cast operator for casting a type_caster to a T +// cast_op operating on an lvalue-reference caster, enables moving only if required by the actual function argument template typename make_caster::template cast_op_type cast_op(make_caster &caster) { return caster.operator typename make_caster::template cast_op_type(); } +// cast_op operating on an rvalue-referenced caster enforces an rvalue-reference for the cast_op type as well template typename make_caster::template cast_op_type::type> cast_op(make_caster &&caster) { - return std::move(caster).operator - typename make_caster::template cast_op_type::type>(); + return caster.operator typename make_caster::template cast_op_type::type>(); } template class type_caster> { @@ -101,7 +102,7 @@ template class type_caster> { } \ operator type*() { return &value; } \ operator type&() { return value; } \ - operator type&&() && { return std::move(value); } \ + operator type&&() { return std::move(value); } \ template using cast_op_type = pybind11::detail::movable_cast_op_type @@ -644,6 +645,10 @@ struct copyable_holder_caster : public type_caster_base { // static_cast works around compiler error with MSVC 17 and CUDA 10.2 // see issue #2180 explicit operator type&() { return *(static_cast(this->value)); } + + // holders only support copying, not moving + template using cast_op_type = detail::cast_op_type; + explicit operator holder_type*() { return std::addressof(holder); } explicit operator holder_type&() { return holder; } @@ -791,8 +796,8 @@ class type_caster::value>> : public pyobject_caste // - type_caster::operator T&() must exist // - the type must be move constructible (obviously) // At run-time: -// - if the type is non-copy-constructible, the object must be the sole owner of the type (i.e. it -// must have ref_count() == 1)h +// - if the type is non-copy-constructible, the object must be the sole owner of the type +// (i.e. it must have ref_count() == 1) // If any of the above are not satisfied, we fall back to copying. template using move_is_plain_type = satisfies_none_of type_caster &load_type(type_ca } return conv; } -// Wrapper around the above that also constructs and returns a type_caster -template make_caster load_type(const handle &handle) { - make_caster conv; - load_type(conv, handle); - return conv; -} PYBIND11_NAMESPACE_END(detail) @@ -866,7 +865,9 @@ T cast(const handle &handle) { using namespace detail; static_assert(!cast_is_temporary_value_reference::value, "Unable to cast type to reference: value is local to type caster"); - return cast_op(load_type(handle)); + make_caster conv; + load_type(conv, handle); + return cast_op(conv); } // pytype -> pytype (calls converting constructor) @@ -902,7 +903,9 @@ detail::enable_if_t::value, T> move(object &&obj) { #endif // Move into a temporary and return that, because the reference may be a local value of `conv` - T ret = std::move(detail::load_type(obj).operator T&()); + detail::make_caster conv; + load_type(conv, obj); + T ret = std::move(conv.operator T &()); return ret; } @@ -1161,7 +1164,7 @@ class argument_loader { template Return call_impl(Func &&f, index_sequence, Guard &&) && { - return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); + return std::forward(f)(cast_op(std::get(argcasters))...); } std::tuple...> argcasters; diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 8ebe39879a..96e0542bf8 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -760,8 +760,7 @@ class type_caster_generic { * Determine suitable casting operator for pointer-or-lvalue-casting type casters. The type caster * needs to provide `operator T*()` and `operator T&()` operators. * - * If the type supports moving the value away via an `operator T&&() &&` method, it should use - * `movable_cast_op_type` instead. + * If the type supports moving, it should use `movable_cast_op_type` instead. */ template using cast_op_type = @@ -769,20 +768,36 @@ using cast_op_type = typename std::add_pointer>::type, typename std::add_lvalue_reference>::type>; +// A by-value argument will use lvalue references by default (triggering copy construction) +// except the type is move-only and thus should use move semantics. +template using cast_op_type_for_byvalue = + conditional_t>::value, + typename std::add_lvalue_reference>::type, + typename std::add_rvalue_reference>::type >; + /** - * Determine suitable casting operator for a type caster with a movable value. Such a type caster - * needs to provide `operator T*()`, `operator T&()`, and `operator T&&() &&`. The latter will be - * called in appropriate contexts where the value can be moved rather than copied. + * Determine suitable casting operator for movable types. + * While C++ decides about copy vs. move semantics based on the type of the passed argument, + * Python doesn't know about lvalue and rvalue references and thus cannot decide based on the + * passed argument. Instead, the wrapper needs to define the desired semantics of argument passing + * explicitly: + * - By specifying an rvalue reference (T&&), move semantics is enforced. + * - By default, lvalue references (T&) are passed, triggering copy semantics if necessary. + * An exception are move-only types: These are passed via move semantics. * - * These operator are automatically provided when using the PYBIND11_TYPE_CASTER macro. + * The corresponding caster needs to provide the following operators: + * `operator T*()`, `operator T&()`, and `operator T&&()`. + * These operators are automatically provided when using the PYBIND11_TYPE_CASTER macro. */ template using movable_cast_op_type = conditional_t::type>::value, typename std::add_pointer>::type, - conditional_t::value, - typename std::add_rvalue_reference>::type, - typename std::add_lvalue_reference>::type>>; + conditional_t::value, + typename std::add_rvalue_reference>::type, + conditional_t::value, + typename std::add_lvalue_reference>::type, + cast_op_type_for_byvalue > > >; // std::is_copy_constructible isn't quite enough: it lets std::vector (and similar) through when // T is non-copyable, but code containing such a copy constructor fails to actually compile. @@ -912,10 +927,12 @@ template class type_caster_base : public type_caster_generic { nullptr, nullptr, holder); } - template using cast_op_type = detail::cast_op_type; + // allow moving of custom types + template using cast_op_type = detail::movable_cast_op_type; operator itype*() { return (type *) value; } operator itype&() { if (!value) throw reference_cast_error(); return *((itype *) value); } + operator itype&&() { if (!value) throw reference_cast_error(); return std::move(*((itype *) value)); } protected: using Constructor = void *(*)(const void *); diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index e8c6f63391..32a38cc0e5 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -346,7 +346,7 @@ struct type_caster::value>> { operator Type*() { return &value; } operator Type&() { return value; } - operator Type&&() && { return std::move(value); } + operator Type&&() { return std::move(value); } template using cast_op_type = movable_cast_op_type; private: diff --git a/tests/test_class.cpp b/tests/test_class.cpp index bd545e8c68..6718d69f4f 100644 --- a/tests/test_class.cpp +++ b/tests/test_class.cpp @@ -421,6 +421,7 @@ TEST_SUBMODULE(class_, m) { // test_exception_rvalue_abort struct PyPrintDestructor { PyPrintDestructor() = default; + PyPrintDestructor(const PyPrintDestructor&) = default; ~PyPrintDestructor() { py::print("Print from destructor"); } diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 322e9bb85d..1876abef6d 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -34,11 +34,10 @@ struct lacking_move_ctor : public empty { template <> lacking_move_ctor empty::instance_ = {}; /* Custom type caster move/copy test classes */ -class MoveOnlyInt { -public: - MoveOnlyInt() { print_default_created(this); } - MoveOnlyInt(int v) : value{std::move(v)} { print_created(this, value); } - MoveOnlyInt(MoveOnlyInt &&m) { print_move_created(this, m.value); std::swap(value, m.value); } +struct MoveOnlyInt { + MoveOnlyInt() : value{42} { print_default_created(this); } + MoveOnlyInt(int v) : value{v} { print_created(this, value); } + MoveOnlyInt(MoveOnlyInt &&m) : value{-1} { print_move_created(this, m.value); std::swap(value, m.value); } MoveOnlyInt &operator=(MoveOnlyInt &&m) { print_move_assigned(this, m.value); std::swap(value, m.value); return *this; } MoveOnlyInt(const MoveOnlyInt &) = delete; MoveOnlyInt &operator=(const MoveOnlyInt &) = delete; @@ -46,11 +45,10 @@ class MoveOnlyInt { int value; }; -class MoveOrCopyInt { -public: - MoveOrCopyInt() { print_default_created(this); } - MoveOrCopyInt(int v) : value{std::move(v)} { print_created(this, value); } - MoveOrCopyInt(MoveOrCopyInt &&m) { print_move_created(this, m.value); std::swap(value, m.value); } +struct MoveOrCopyInt { + MoveOrCopyInt() : value{42} { print_default_created(this); } + MoveOrCopyInt(int v) : value{v} { print_created(this, value); } + MoveOrCopyInt(MoveOrCopyInt &&m) : value{-1} { print_move_created(this, m.value); std::swap(value, m.value); } MoveOrCopyInt &operator=(MoveOrCopyInt &&m) { print_move_assigned(this, m.value); std::swap(value, m.value); return *this; } MoveOrCopyInt(const MoveOrCopyInt &c) { print_copy_created(this, c.value); value = c.value; } MoveOrCopyInt &operator=(const MoveOrCopyInt &c) { print_copy_assigned(this, c.value); value = c.value; return *this; } @@ -58,16 +56,16 @@ class MoveOrCopyInt { int value; }; -class CopyOnlyInt { -public: - CopyOnlyInt() { print_default_created(this); } - CopyOnlyInt(int v) : value{std::move(v)} { print_created(this, value); } +struct CopyOnlyInt { + CopyOnlyInt() : value{42} { print_default_created(this); } + CopyOnlyInt(int v) : value{v} { print_created(this, value); } CopyOnlyInt(const CopyOnlyInt &c) { print_copy_created(this, c.value); value = c.value; } CopyOnlyInt &operator=(const CopyOnlyInt &c) { print_copy_assigned(this, c.value); value = c.value; return *this; } ~CopyOnlyInt() { print_destroyed(this); } int value; }; + PYBIND11_NAMESPACE_BEGIN(pybind11) PYBIND11_NAMESPACE_BEGIN(detail) template <> struct type_caster { @@ -113,10 +111,10 @@ TEST_SUBMODULE(copy_move_policies, m) { // test_move_and_copy_casts m.def("move_and_copy_casts", [](py::object o) { int r = 0; - r += py::cast(o).value; /* moves */ + r += py::cast(o).value; /* copies */ r += py::cast(o).value; /* moves */ r += py::cast(o).value; /* copies */ - auto m1(py::cast(o)); /* moves */ + auto m1(py::cast(o)); /* copies */ auto m2(py::cast(o)); /* moves */ auto m3(py::cast(o)); /* copies */ r += m1.value + m2.value + m3.value; @@ -126,7 +124,8 @@ TEST_SUBMODULE(copy_move_policies, m) { // test_move_and_copy_loads m.def("move_only", [](MoveOnlyInt m) { return m.value; }); - m.def("move_or_copy", [](MoveOrCopyInt m) { return m.value; }); + m.def("move", [](MoveOrCopyInt&& m) { return m.value; }); + m.def("copy", [](MoveOrCopyInt m) { return m.value; }); m.def("copy_only", [](CopyOnlyInt m) { return m.value; }); m.def("move_pair", [](std::pair p) { return p.first.value + p.second.value; @@ -156,18 +155,40 @@ TEST_SUBMODULE(copy_move_policies, m) { d["CopyOnlyInt"] = py::cast(co, py::return_value_policy::reference); return d; }); + + // test_move_copy_class_loads: similar tests as above, but with types exposed to python + struct MoveOnlyIntC : MoveOnlyInt { using MoveOnlyInt::MoveOnlyInt; }; + py::class_(m, "MoveOnlyInt") + .def(py::init()) + .def_readonly("value", &MoveOnlyIntC::value) + .def_static("val", [](MoveOnlyIntC v) { return v.value; }) + .def_static("rref", [](MoveOnlyIntC&& v) { MoveOnlyIntC sink(std::move(v)); return sink.value; }) + .def_static("lref", [](const MoveOnlyIntC& v) { return v.value; }) + ; + struct MoveOrCopyIntC : MoveOrCopyInt { using MoveOrCopyInt::MoveOrCopyInt; }; + py::class_(m, "MoveOrCopyInt") + .def(py::init()) + .def_readonly("value", &MoveOrCopyIntC::value) + .def_static("val", [](MoveOrCopyIntC v) { return v.value; }) + .def_static("rref", [](MoveOrCopyIntC&& v) { MoveOrCopyIntC sink(std::move(v)); return sink.value; }) + .def_static("lref", [](const MoveOrCopyIntC& v) { return v.value; }) + ; + struct CopyOnlyIntC : CopyOnlyInt { using CopyOnlyInt::CopyOnlyInt; }; + py::class_(m, "CopyOnlyInt") + .def(py::init()) + .def_readonly("value", &CopyOnlyIntC::value) + .def_static("val", [](CopyOnlyIntC v) { return v.value; }) + .def_static("rref", [](CopyOnlyIntC&& v) { CopyOnlyIntC sink(std::move(v)); return sink.value; }) + .def_static("lref", [](const CopyOnlyIntC& v) { return v.value; }) + ; + #ifdef PYBIND11_HAS_OPTIONAL // test_move_and_copy_load_optional m.attr("has_optional") = true; - m.def("move_optional", [](std::optional o) { - return o->value; - }); - m.def("move_or_copy_optional", [](std::optional o) { - return o->value; - }); - m.def("copy_optional", [](std::optional o) { - return o->value; - }); + m.def("move_optional", [](std::optional o) { return o->value; }); + m.def("mc_move_optional", [](std::optional&& o) { return o->value; }); + m.def("mc_copy_optional", [](std::optional o) { return o->value; }); + m.def("copy_optional", [](std::optional o) { return o->value; }); m.def("move_optional_tuple", [](std::optional> x) { return std::get<0>(*x).value + std::get<1>(*x).value + std::get<2>(*x).value; }); diff --git a/tests/test_copy_move.py b/tests/test_copy_move.py index 1d98952200..b5a1d808d8 100644 --- a/tests/test_copy_move.py +++ b/tests/test_copy_move.py @@ -20,22 +20,21 @@ def test_move_and_copy_casts(): cstats = m.move_and_copy_cstats() c_m, c_mc, c_c = ( - cstats["MoveOnlyInt"], - cstats["MoveOrCopyInt"], - cstats["CopyOnlyInt"], + cstats[name] for name in ["MoveOnlyInt", "MoveOrCopyInt", "CopyOnlyInt"] ) # The type move constructions/assignments below each get incremented: the move assignment comes # from the type_caster load; the move construction happens when extracting that via a cast or # loading into an argument. - assert m.move_and_copy_casts(3) == 18 + assert m.move_and_copy_casts(3) == 6 * 3 assert c_m.copy_assignments + c_m.copy_constructions == 0 assert c_m.move_assignments == 2 assert c_m.move_constructions >= 2 assert c_mc.alive() == 0 - assert c_mc.copy_assignments + c_mc.copy_constructions == 0 + assert c_mc.copy_assignments == 0 + assert c_mc.copy_constructions == 2 assert c_mc.move_assignments == 2 - assert c_mc.move_constructions >= 2 + assert c_mc.move_constructions == 0 assert c_c.alive() == 0 assert c_c.copy_assignments == 2 assert c_c.copy_constructions >= 2 @@ -48,47 +47,76 @@ def test_move_and_copy_loads(): cstats = m.move_and_copy_cstats() c_m, c_mc, c_c = ( - cstats["MoveOnlyInt"], - cstats["MoveOrCopyInt"], - cstats["CopyOnlyInt"], + cstats[name] for name in ["MoveOnlyInt", "MoveOrCopyInt", "CopyOnlyInt"] ) assert m.move_only(10) == 10 # 1 move, c_m - assert m.move_or_copy(11) == 11 # 1 move, c_mc + assert m.move(10) == 10 # 0 move (just ref), c_mc + assert m.copy(11) == 11 # 1 copy, c_mc assert m.copy_only(12) == 12 # 1 copy, c_c - assert m.move_pair((13, 14)) == 27 # 1 c_m move, 1 c_mc move - assert m.move_tuple((15, 16, 17)) == 48 # 2 c_m moves, 1 c_mc move + assert m.move_pair((13, 14)) == 27 # 1 c_m move, 1 c_mc copy + assert m.move_tuple((15, 16, 17)) == 48 # 2 c_m moves, 1 c_mc copy assert m.copy_tuple((18, 19)) == 37 # 2 c_c copies - # Direct constructions: 2 c_m moves, 2 c_mc moves, 1 c_c copy + # Direct constructions: 2 c_m moves, 2 c_mc copies, 1 c_c copy # Extra moves/copies when moving pairs/tuples: 3 c_m, 3 c_mc, 2 c_c assert m.move_copy_nested((1, ((2, 3, (4,)), 5))) == 15 assert c_m.copy_assignments + c_m.copy_constructions == 0 assert c_m.move_assignments == 6 assert c_m.move_constructions == 9 - assert c_mc.copy_assignments + c_mc.copy_constructions == 0 - assert c_mc.move_assignments == 5 - assert c_mc.move_constructions == 8 + assert c_mc.copy_assignments == 0 + assert c_mc.copy_constructions == 5 + assert c_mc.move_assignments == 6 + assert c_mc.move_constructions == 3 assert c_c.copy_assignments == 4 assert c_c.copy_constructions == 6 assert c_m.alive() + c_mc.alive() + c_c.alive() == 0 +def test_move_copy_class_loads(): + """Call some functions that load custom type arguments and count the number of moves/copies""" + + cs = m.move_and_copy_cstats()["MoveOnlyInt"] + o = m.MoveOnlyInt(3) + assert m.MoveOnlyInt.val(o) == 3 + assert o.value == -1 # value becomes -1 after moving + o = m.MoveOnlyInt(4) + assert m.MoveOnlyInt.lref(o) == 4 + assert m.MoveOnlyInt.rref(o) == 4 + assert o.value == -1 # value becomes -1 after moving + assert (cs.copy_constructions, cs.move_constructions) == (0, 2) + + cs = m.move_and_copy_cstats()["MoveOrCopyInt"] + o = m.MoveOrCopyInt(3) + assert m.MoveOrCopyInt.val(o) == 3 + assert m.MoveOrCopyInt.lref(o) == 3 + assert m.MoveOrCopyInt.rref(o) == 3 + assert o.value == -1 # value becomes -1 after moving + assert (cs.copy_constructions, cs.move_constructions) == (1, 1) + + cs = m.move_and_copy_cstats()["CopyOnlyInt"] + o = m.CopyOnlyInt(3) + assert m.CopyOnlyInt.val(o) == 3 + assert m.CopyOnlyInt.lref(o) == 3 + assert m.CopyOnlyInt.rref(o) == 3 # copies as well + assert o.value == 3 # value hasn't change + assert (cs.copy_constructions, cs.move_constructions) == (2, 0) + + @pytest.mark.skipif(not m.has_optional, reason="no ") def test_move_and_copy_load_optional(): """Tests move/copy loads of std::optional arguments""" cstats = m.move_and_copy_cstats() c_m, c_mc, c_c = ( - cstats["MoveOnlyInt"], - cstats["MoveOrCopyInt"], - cstats["CopyOnlyInt"], + cstats[name] for name in ["MoveOnlyInt", "MoveOrCopyInt", "CopyOnlyInt"] ) # The extra move/copy constructions below come from the std::optional move (which has to move # its arguments): - assert m.move_optional(10) == 10 # c_m: 1 move assign, 2 move construct - assert m.move_or_copy_optional(11) == 11 # c_mc: 1 move assign, 2 move construct + assert m.move_optional(10) == 10 # c_m: 1 move assign, +1 move construct + assert m.mc_move_optional(10) == 10 # c_mc: 1 move assign, +0 move construct + assert m.mc_copy_optional(11) == 11 # c_mc: 1 move assign, 1 copy construct assert m.copy_optional(12) == 12 # c_c: 1 copy assign, 2 copy construct # 1 move assign + move construct moves each of c_m, c_mc, 1 c_c copy # +1 move/copy construct each from moving the tuple @@ -98,9 +126,10 @@ def test_move_and_copy_load_optional(): assert c_m.copy_assignments + c_m.copy_constructions == 0 assert c_m.move_assignments == 2 assert c_m.move_constructions == 5 - assert c_mc.copy_assignments + c_mc.copy_constructions == 0 - assert c_mc.move_assignments == 2 - assert c_mc.move_constructions == 5 + assert c_mc.copy_assignments == 0 + assert c_mc.copy_constructions == 2 + assert c_mc.move_assignments == 3 + assert c_mc.move_constructions == 4 assert c_c.copy_assignments == 2 assert c_c.copy_constructions == 5 assert c_m.alive() + c_mc.alive() + c_c.alive() == 0 diff --git a/tests/test_kwargs_and_defaults.py b/tests/test_kwargs_and_defaults.py index 12fe705b4d..0cd9a5f91d 100644 --- a/tests/test_kwargs_and_defaults.py +++ b/tests/test_kwargs_and_defaults.py @@ -244,7 +244,7 @@ def test_args_refcount(): myval = 54321 expected = refcount(myval) assert m.arg_refcount_h(myval) == expected - assert m.arg_refcount_o(myval) == expected + 1 + assert m.arg_refcount_o(myval) == expected + 2 assert m.arg_refcount_h(myval) == expected assert refcount(myval) == expected @@ -280,6 +280,6 @@ def test_args_refcount(): # for the `py::args`; in the previous case, we could simply inc_ref and pass on Python's input # tuple without having to inc_ref the individual elements, but here we can't, hence the extra # refs. - assert m.mixed_args_refcount(myval, myval, myval) == (exp3 + 3, exp3 + 3, exp3 + 3) + assert m.mixed_args_refcount(myval, myval, myval) == (exp3 + 4, exp3 + 4, exp3 + 4) assert m.class_default_argument() == "" diff --git a/tests/test_numpy_array.cpp b/tests/test_numpy_array.cpp index dca7145f9d..8b9770d63b 100644 --- a/tests/test_numpy_array.cpp +++ b/tests/test_numpy_array.cpp @@ -206,6 +206,7 @@ TEST_SUBMODULE(numpy_array, sm) { struct ArrayClass { int data[2] = { 1, 2 }; ArrayClass() { py::print("ArrayClass()"); } + ArrayClass(const ArrayClass&) = default; ~ArrayClass() { py::print("~ArrayClass()"); } }; py::class_(sm, "ArrayClass") diff --git a/tests/test_smart_ptr.cpp b/tests/test_smart_ptr.cpp index e0af249790..cffffde6da 100644 --- a/tests/test_smart_ptr.cpp +++ b/tests/test_smart_ptr.cpp @@ -161,6 +161,7 @@ class MyObject4b : public MyObject4a { class MyObject5 { // managed by huge_unique_ptr public: MyObject5(int value) : value{value} { print_created(this); } + MyObject5(const MyObject5&) = default; ~MyObject5() { print_destroyed(this); } int value; }; @@ -220,6 +221,7 @@ struct TypeForHolderWithAddressOf { // test_move_only_holder_with_addressof_operator struct TypeForMoveOnlyHolderWithAddressOf { TypeForMoveOnlyHolderWithAddressOf(int value) : value{value} { print_created(this); } + TypeForMoveOnlyHolderWithAddressOf(const TypeForMoveOnlyHolderWithAddressOf&) = default; ~TypeForMoveOnlyHolderWithAddressOf() { print_destroyed(this); } std::string toString() const { return "MoveOnlyHolderWithAddressOf[" + std::to_string(value) + "]";