diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index bca4e85e3..4c8512535 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -381,11 +381,33 @@ protected: object temp; }; -/* Determine suitable casting operator */ +/** + * 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. + */ template -using cast_op_type = typename std::conditional::type>::value, - typename std::add_pointer>::type, - typename std::add_lvalue_reference>::type>::type; +using cast_op_type = + conditional_t::type>::value, + typename std::add_pointer>::type, + typename std::add_lvalue_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. + * + * These operator 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>>; // 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. @@ -462,7 +484,7 @@ public: nullptr, nullptr, holder); } - template using cast_op_type = pybind11::detail::cast_op_type; + template using cast_op_type = cast_op_type; operator itype*() { return (type *) value; } operator itype&() { if (!value) throw reference_cast_error(); return *((itype *) value); } @@ -498,8 +520,10 @@ template using make_caster = type_caster>; template typename make_caster::template cast_op_type cast_op(make_caster &caster) { return caster.operator typename make_caster::template cast_op_type(); } -template typename make_caster::template cast_op_type cast_op(make_caster &&caster) { - return cast_op(caster); +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>(); } template class type_caster> : public type_caster_base { @@ -522,7 +546,8 @@ public: } \ operator type*() { return &value; } \ operator type&() { return value; } \ - template using cast_op_type = pybind11::detail::cast_op_type<_T> + operator type&&() && { return std::move(value); } \ + template using cast_op_type = pybind11::detail::movable_cast_op_type<_T> template using is_std_char_type = any_of< @@ -892,9 +917,8 @@ public: template using cast_op_type = type; - operator type() { - return type(cast_op(first), cast_op(second)); - } + operator type() & { return type(cast_op(first), cast_op(second)); } + operator type() && { return type(cast_op(std::move(first)), cast_op(std::move(second))); } protected: make_caster first; make_caster second; @@ -925,17 +949,21 @@ public: template using cast_op_type = type; - operator type() { return implicit_cast(indices{}); } + operator type() & { return implicit_cast(indices{}); } + operator type() && { return std::move(*this).implicit_cast(indices{}); } protected: template - type implicit_cast(index_sequence) { return type(cast_op(std::get(value))...); } + type implicit_cast(index_sequence) & { return type(cast_op(std::get(subcasters))...); } + template + type implicit_cast(index_sequence) && { return type(cast_op(std::move(std::get(subcasters)))...); } + static constexpr bool load_impl(const sequence &, bool, index_sequence<>) { return true; } template bool load_impl(const sequence &seq, bool convert, index_sequence) { - for (bool r : {std::get(value).load(seq[Is], convert)...}) + for (bool r : {std::get(subcasters).load(seq[Is], convert)...}) if (!r) return false; return true; @@ -960,7 +988,7 @@ protected: return result.release(); } - std::tuple...> value; + std::tuple...> subcasters; }; /// Helper class which abstracts away certain actions. Users can provide specializations for @@ -1465,13 +1493,13 @@ public: } template - enable_if_t::value, Return> call(Func &&f) { - return call_impl(std::forward(f), indices{}, Guard{}); + enable_if_t::value, Return> call(Func &&f) && { + return std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); } template - enable_if_t::value, void_type> call(Func &&f) { - call_impl(std::forward(f), indices{}, Guard{}); + enable_if_t::value, void_type> call(Func &&f) && { + std::move(*this).template call_impl(std::forward(f), indices{}, Guard{}); return void_type(); } @@ -1481,7 +1509,7 @@ private: template bool load_impl_sequence(function_call &call, index_sequence) { - for (bool r : {std::get(value).load(call.args[Is], call.args_convert[Is])...}) + for (bool r : {std::get(argcasters).load(call.args[Is], call.args_convert[Is])...}) if (!r) return false; return true; @@ -1489,10 +1517,10 @@ private: template Return call_impl(Func &&f, index_sequence, Guard &&) { - return std::forward(f)(cast_op(std::get(value))...); + return std::forward(f)(cast_op(std::move(std::get(argcasters)))...); } - std::tuple...> value; + std::tuple...> argcasters; }; /// Helper class which collects only positional arguments for a Python function call. diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index fc074dbcd..8ceb94101 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -341,7 +341,8 @@ public: operator Type*() { return &value; } operator Type&() { return value; } - template using cast_op_type = cast_op_type; + operator Type&&() && { return std::move(value); } + template using cast_op_type = movable_cast_op_type; private: Type value; diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 1f36308c9..6ac8edc2b 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -150,8 +150,8 @@ protected: using Guard = detail::extract_guard_t; /* Perform the function call */ - handle result = cast_out::cast(args_converter.template call(cap->f), - policy, call.parent); + handle result = cast_out::cast( + std::move(args_converter).template call(cap->f), policy, call.parent); /* Invoke call policy post-call hook */ detail::process_attributes::postcall(call, result); diff --git a/include/pybind11/stl.h b/include/pybind11/stl.h index f37dd795e..495f85d3d 100644 --- a/include/pybind11/stl.h +++ b/include/pybind11/stl.h @@ -60,11 +60,11 @@ template struct set_caster { return false; auto s = reinterpret_borrow(src); value.clear(); - key_conv conv; for (auto entry : s) { + key_conv conv; if (!conv.load(entry, convert)) return false; - value.insert(cast_op(conv)); + value.insert(cast_op(std::move(conv))); } return true; } @@ -90,14 +90,14 @@ template struct map_caster { if (!isinstance(src)) return false; auto d = reinterpret_borrow(src); - key_conv kconv; - value_conv vconv; value.clear(); for (auto it : d) { + key_conv kconv; + value_conv vconv; if (!kconv.load(it.first.ptr(), convert) || !vconv.load(it.second.ptr(), convert)) return false; - value.emplace(cast_op(kconv), cast_op(vconv)); + value.emplace(cast_op(std::move(kconv)), cast_op(std::move(vconv))); } return true; } @@ -124,13 +124,13 @@ template struct list_caster { if (!isinstance(src)) return false; auto s = reinterpret_borrow(src); - value_conv conv; value.clear(); reserve_maybe(s, &value); for (auto it : s) { + value_conv conv; if (!conv.load(it, convert)) return false; - value.push_back(cast_op(conv)); + value.push_back(cast_op(std::move(conv))); } return true; } @@ -185,12 +185,12 @@ public: auto l = reinterpret_borrow(src); if (!require_size(l.size())) return false; - value_conv conv; size_t ctr = 0; for (auto it : l) { + value_conv conv; if (!conv.load(it, convert)) return false; - value[ctr++] = cast_op(conv); + value[ctr++] = cast_op(std::move(conv)); } return true; } @@ -249,7 +249,7 @@ template struct optional_caster { if (!inner_caster.load(src, convert)) return false; - value.emplace(cast_op(inner_caster)); + value.emplace(cast_op(std::move(inner_caster))); return true; } diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index eae177ea5..36988c360 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -33,7 +33,7 @@ set(PYBIND11_TEST_FILES test_chrono.cpp test_class_args.cpp test_constants_and_functions.cpp - test_copy_move_policies.cpp + test_copy_move.cpp test_docstring_options.cpp test_eigen.cpp test_enum.cpp diff --git a/tests/test_copy_move.cpp b/tests/test_copy_move.cpp new file mode 100644 index 000000000..898fc39d1 --- /dev/null +++ b/tests/test_copy_move.cpp @@ -0,0 +1,177 @@ +/* + tests/test_copy_move_policies.cpp -- 'copy' and 'move' return value policies + and related tests + + Copyright (c) 2016 Ben North + + All rights reserved. Use of this source code is governed by a + BSD-style license that can be found in the LICENSE file. +*/ + +#include "pybind11_tests.h" +#include "constructor_stats.h" +#include + +template +struct empty { + static const derived& get_one() { return instance_; } + static derived instance_; +}; + +struct lacking_copy_ctor : public empty { + lacking_copy_ctor() {} + lacking_copy_ctor(const lacking_copy_ctor& other) = delete; +}; + +template <> lacking_copy_ctor empty::instance_ = {}; + +struct lacking_move_ctor : public empty { + lacking_move_ctor() {} + lacking_move_ctor(const lacking_move_ctor& other) = delete; + lacking_move_ctor(lacking_move_ctor&& other) = delete; +}; + +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); } + 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; + ~MoveOnlyInt() { print_destroyed(this); } + + 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); } + 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; } + ~MoveOrCopyInt() { print_destroyed(this); } + + int value; +}; +class CopyOnlyInt { +public: + CopyOnlyInt() { print_default_created(this); } + CopyOnlyInt(int v) : value{std::move(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; +}; +namespace pybind11 { namespace detail { +template <> struct type_caster { + PYBIND11_TYPE_CASTER(MoveOnlyInt, _("MoveOnlyInt")); + bool load(handle src, bool) { value = MoveOnlyInt(src.cast()); return true; } + static handle cast(const MoveOnlyInt &m, return_value_policy r, handle p) { return pybind11::cast(m.value, r, p); } +}; + +template <> struct type_caster { + PYBIND11_TYPE_CASTER(MoveOrCopyInt, _("MoveOrCopyInt")); + bool load(handle src, bool) { value = MoveOrCopyInt(src.cast()); return true; } + static handle cast(const MoveOrCopyInt &m, return_value_policy r, handle p) { return pybind11::cast(m.value, r, p); } +}; + +template <> struct type_caster { +protected: + CopyOnlyInt value; +public: + static PYBIND11_DESCR name() { return _("CopyOnlyInt"); } + bool load(handle src, bool) { value = CopyOnlyInt(src.cast()); return true; } + static handle cast(const CopyOnlyInt &m, return_value_policy r, handle p) { return pybind11::cast(m.value, r, p); } + static handle cast(const CopyOnlyInt *src, return_value_policy policy, handle parent) { + if (!src) return none().release(); + return cast(*src, policy, parent); + } + operator CopyOnlyInt*() { return &value; } + operator CopyOnlyInt&() { return value; } + template using cast_op_type = pybind11::detail::cast_op_type; +}; +}} + + +test_initializer copy_move_policies([](py::module &m) { + py::class_(m, "lacking_copy_ctor") + .def_static("get_one", &lacking_copy_ctor::get_one, + py::return_value_policy::copy); + py::class_(m, "lacking_move_ctor") + .def_static("get_one", &lacking_move_ctor::get_one, + py::return_value_policy::move); + + m.def("move_only", [](MoveOnlyInt m) { + return m.value; + }); + m.def("move_or_copy", [](MoveOrCopyInt m) { + return m.value; + }); + m.def("copy_only", [](CopyOnlyInt m) { + return m.value; + }); + m.def("move_and_copy_casts", [](py::object o) { + int r = 0; + r += py::cast(o).value; /* moves */ + r += py::cast(o).value; /* moves */ + r += py::cast(o).value; /* copies */ + MoveOrCopyInt m1(py::cast(o)); /* moves */ + MoveOnlyInt m2(py::cast(o)); /* moves */ + CopyOnlyInt m3(py::cast(o)); /* copies */ + r += m1.value + m2.value + m3.value; + + return r; + }); + m.def("move_pair", [](std::pair p) { + return p.first.value + p.second.value; + }); + m.def("move_tuple", [](std::tuple t) { + return std::get<0>(t).value + std::get<1>(t).value + std::get<2>(t).value; + }); + m.def("copy_tuple", [](std::tuple t) { + return std::get<0>(t).value + std::get<1>(t).value; + }); + m.def("move_copy_nested", [](std::pair>, MoveOrCopyInt>> x) { + return x.first.value + std::get<0>(x.second.first).value + std::get<1>(x.second.first).value + + std::get<0>(std::get<2>(x.second.first)).value + x.second.second.value; + }); + m.def("move_and_copy_cstats", []() { + ConstructorStats::gc(); + // Reset counts to 0 so that previous tests don't affect later ones: + auto &mc = ConstructorStats::get(); + mc.move_assignments = mc.move_constructions = mc.copy_assignments = mc.copy_constructions = 0; + auto &mo = ConstructorStats::get(); + mo.move_assignments = mo.move_constructions = mo.copy_assignments = mo.copy_constructions = 0; + auto &co = ConstructorStats::get(); + co.move_assignments = co.move_constructions = co.copy_assignments = co.copy_constructions = 0; + py::dict d; + d["MoveOrCopyInt"] = py::cast(mc, py::return_value_policy::reference); + d["MoveOnlyInt"] = py::cast(mo, py::return_value_policy::reference); + d["CopyOnlyInt"] = py::cast(co, py::return_value_policy::reference); + return d; + }); +#ifdef PYBIND11_HAS_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_tuple", [](std::optional> x) { + return std::get<0>(*x).value + std::get<1>(*x).value + std::get<2>(*x).value; + }); +#else + m.attr("has_optional") = false; +#endif + +}); diff --git a/tests/test_copy_move.py b/tests/test_copy_move.py new file mode 100644 index 000000000..d6479c588 --- /dev/null +++ b/tests/test_copy_move.py @@ -0,0 +1,100 @@ +import pytest +from pybind11_tests import has_optional + + +def test_lacking_copy_ctor(): + from pybind11_tests import lacking_copy_ctor + with pytest.raises(RuntimeError) as excinfo: + lacking_copy_ctor.get_one() + assert "the object is non-copyable!" in str(excinfo.value) + + +def test_lacking_move_ctor(): + from pybind11_tests import lacking_move_ctor + with pytest.raises(RuntimeError) as excinfo: + lacking_move_ctor.get_one() + assert "the object is neither movable nor copyable!" in str(excinfo.value) + + +def test_move_and_copy_casts(): + """Cast some values in C++ via custom type casters and count the number of moves/copies.""" + from pybind11_tests import move_and_copy_casts, move_and_copy_cstats + + cstats = move_and_copy_cstats() + c_m, c_mc, c_c = cstats["MoveOnlyInt"], cstats["MoveOrCopyInt"], cstats["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 move_and_copy_casts(3) == 18 + 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.move_assignments == 2 + assert c_mc.move_constructions == 2 + assert c_c.alive() == 0 + assert c_c.copy_assignments == 2 + assert c_c.copy_constructions == 2 + assert c_m.alive() + c_mc.alive() + c_c.alive() == 0 + + +def test_move_and_copy_loads(): + """Call some functions that load arguments via custom type casters and count the number of + moves/copies.""" + from pybind11_tests import (move_and_copy_cstats, move_only, move_or_copy, copy_only, + move_pair, move_tuple, copy_tuple, move_copy_nested) + + cstats = move_and_copy_cstats() + c_m, c_mc, c_c = cstats["MoveOnlyInt"], cstats["MoveOrCopyInt"], cstats["CopyOnlyInt"] + + assert move_only(10) == 10 # 1 move, c_m + assert move_or_copy(11) == 11 # 1 move, c_mc + assert copy_only(12) == 12 # 1 copy, c_c + assert move_pair((13, 14)) == 27 # 1 c_m move, 1 c_mc move + assert move_tuple((15, 16, 17)) == 48 # 2 c_m moves, 1 c_mc move + assert copy_tuple((18, 19)) == 37 # 2 c_c copies + # Direct constructions: 2 c_m moves, 2 c_mc moves, 1 c_c copy + # Extra moves/copies when moving pairs/tuples: 3 c_m, 3 c_mc, 2 c_c + assert 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_c.copy_assignments == 4 + assert c_c.copy_constructions == 6 + assert c_m.alive() + c_mc.alive() + c_c.alive() == 0 + + +@pytest.mark.skipif(not has_optional, reason='no ') +def test_move_and_copy_load_optional(): + """Tests move/copy loads of std::optional arguments""" + from pybind11_tests import (move_and_copy_cstats, move_optional, move_or_copy_optional, + copy_optional, move_optional_tuple) + + cstats = move_and_copy_cstats() + c_m, c_mc, c_c = cstats["MoveOnlyInt"], cstats["MoveOrCopyInt"], cstats["CopyOnlyInt"] + + # The extra move/copy constructions below come from the std::optional move (which has to move + # its arguments): + assert move_optional(10) == 10 # c_m: 1 move assign, 2 move construct + assert move_or_copy_optional(11) == 11 # c_mc: 1 move assign, 2 move construct + assert 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 + # +1 move/copy construct each from moving the optional (which moves the tuple again) + assert move_optional_tuple((3, 4, 5)) == 12 + + 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_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_copy_move_policies.cpp b/tests/test_copy_move_policies.cpp deleted file mode 100644 index 6f7907c1f..000000000 --- a/tests/test_copy_move_policies.cpp +++ /dev/null @@ -1,41 +0,0 @@ -/* - tests/test_copy_move_policies.cpp -- 'copy' and 'move' - return value policies - - Copyright (c) 2016 Ben North - - All rights reserved. Use of this source code is governed by a - BSD-style license that can be found in the LICENSE file. -*/ - -#include "pybind11_tests.h" - -template -struct empty { - static const derived& get_one() { return instance_; } - static derived instance_; -}; - -struct lacking_copy_ctor : public empty { - lacking_copy_ctor() {} - lacking_copy_ctor(const lacking_copy_ctor& other) = delete; -}; - -template <> lacking_copy_ctor empty::instance_ = {}; - -struct lacking_move_ctor : public empty { - lacking_move_ctor() {} - lacking_move_ctor(const lacking_move_ctor& other) = delete; - lacking_move_ctor(lacking_move_ctor&& other) = delete; -}; - -template <> lacking_move_ctor empty::instance_ = {}; - -test_initializer copy_move_policies([](py::module &m) { - py::class_(m, "lacking_copy_ctor") - .def_static("get_one", &lacking_copy_ctor::get_one, - py::return_value_policy::copy); - py::class_(m, "lacking_move_ctor") - .def_static("get_one", &lacking_move_ctor::get_one, - py::return_value_policy::move); -}); diff --git a/tests/test_copy_move_policies.py b/tests/test_copy_move_policies.py deleted file mode 100644 index edcf38075..000000000 --- a/tests/test_copy_move_policies.py +++ /dev/null @@ -1,15 +0,0 @@ -import pytest - - -def test_lacking_copy_ctor(): - from pybind11_tests import lacking_copy_ctor - with pytest.raises(RuntimeError) as excinfo: - lacking_copy_ctor.get_one() - assert "the object is non-copyable!" in str(excinfo.value) - - -def test_lacking_move_ctor(): - from pybind11_tests import lacking_move_ctor - with pytest.raises(RuntimeError) as excinfo: - lacking_move_ctor.get_one() - assert "the object is neither movable nor copyable!" in str(excinfo.value)