From 6f4c8d483450993d3f82169e98c83ce5eb9b6279 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 9 Nov 2020 07:58:38 +0100 Subject: [PATCH 1/6] Add tests for custom-type rvalue function arguments --- tests/test_copy_move.cpp | 53 +++++++++++++++++++++++++++++----------- tests/test_copy_move.py | 42 ++++++++++++++++++++++++------- 2 files changed, 72 insertions(+), 23 deletions(-) diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 322e9bb85d..40ab421f80 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 { @@ -156,6 +154,33 @@ 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; diff --git a/tests/test_copy_move.py b/tests/test_copy_move.py index 1d98952200..efbfc3a568 100644 --- a/tests/test_copy_move.py +++ b/tests/test_copy_move.py @@ -20,9 +20,7 @@ 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 @@ -48,9 +46,7 @@ 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 @@ -74,15 +70,43 @@ def test_move_and_copy_loads(): 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 From 445d5987d3eb10f89ecef3ac1ea60f26e0a06750 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 30 Dec 2019 16:30:03 +0100 Subject: [PATCH 2/6] Allow rvalue references of custom types --- include/pybind11/cast.h | 4 ++++ include/pybind11/detail/type_caster_base.h | 4 +++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 0ad10e9a93..1d89489565 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -644,6 +644,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; } diff --git a/include/pybind11/detail/type_caster_base.h b/include/pybind11/detail/type_caster_base.h index 8ebe39879a..9ef49dec8d 100644 --- a/include/pybind11/detail/type_caster_base.h +++ b/include/pybind11/detail/type_caster_base.h @@ -912,10 +912,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 *); From 396cde5dcd72372a485fc204045dadb220a808ca Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 30 Dec 2019 16:30:03 +0100 Subject: [PATCH 3/6] Fix copying/moving in test_virtual_functions Don't enforce moving by passing the caster as an rvalue to cast_op(). --- include/pybind11/cast.h | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 1d89489565..c95c56b85a 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -855,12 +855,6 @@ template 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) @@ -870,7 +864,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) @@ -906,7 +902,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; } From b4dd7c437943c1133e035a5e5bb57d14e4572744 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Mon, 9 Nov 2020 11:00:28 +0100 Subject: [PATCH 4/6] Differentiate between copying and moving of custom types based on the function signature So far, using the movable_cast_op_type, always enabled move semantics, even when copy semantics would be expected from the function signature. 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 explicitly define the desired semantics of argument passing as follows: - f(T&) passes an lvalue reference - f(T&&) passes an rvalue reference - f(T) passes an lvalue reference by default (triggering copy construction) but falls back to an rvalue reference if copying is not supported (thus triggering move construction) This commit essentially makes call_impl() pass the argument casters to cast_op() by lvalue references, thus enabling the distinction between copy and move semantics based on the argument's type. --- include/pybind11/cast.h | 15 +++++----- include/pybind11/detail/type_caster_base.h | 35 +++++++++++++++------- include/pybind11/eigen.h | 2 +- 3 files changed, 34 insertions(+), 18 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index c95c56b85a..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 @@ -795,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 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 9ef49dec8d..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. @@ -917,7 +932,7 @@ template class type_caster_base : public type_caster_generic { 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)); } + 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: From aa7d99e79da773b65c290573bffd41ff887bc588 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Thu, 4 Mar 2021 19:58:57 +0100 Subject: [PATCH 5/6] Adapt ConstructorStats of unittests As we prefer copying over moving now (if not explicitly asked for otherwise), the constructor stats shift from moving towards copying. --- tests/test_copy_move.cpp | 20 +++++++----------- tests/test_copy_move.py | 35 ++++++++++++++++++------------- tests/test_kwargs_and_defaults.py | 4 ++-- 3 files changed, 30 insertions(+), 29 deletions(-) diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp index 40ab421f80..1876abef6d 100644 --- a/tests/test_copy_move.cpp +++ b/tests/test_copy_move.cpp @@ -111,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; @@ -124,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; @@ -184,15 +185,10 @@ TEST_SUBMODULE(copy_move_policies, m) { #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 efbfc3a568..b5a1d808d8 100644 --- a/tests/test_copy_move.py +++ b/tests/test_copy_move.py @@ -26,14 +26,15 @@ def test_move_and_copy_casts(): # 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 @@ -50,21 +51,23 @@ def test_move_and_copy_loads(): ) 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 @@ -111,8 +114,9 @@ def test_move_and_copy_load_optional(): # 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 @@ -122,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() == "" From b000987799aa9126139300ff814f20b968ac47f0 Mon Sep 17 00:00:00 2001 From: Robert Haschke Date: Sat, 6 Mar 2021 16:39:51 +0100 Subject: [PATCH 6/6] Fix broken CI clang 3.x requires declaration of default copy constructors, due to use of std::is_copy_constructible. --- tests/test_class.cpp | 1 + tests/test_numpy_array.cpp | 1 + tests/test_smart_ptr.cpp | 2 ++ 3 files changed, 4 insertions(+) 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_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) + "]";